Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11298004:
alternate ntp: add "Recent Tabs" submenu to wrench menu (Closed)
|
Created:
8 years, 1 month ago by kuan Modified:
8 years, 1 month ago CC:
chromium-reviews, tfarina, mazda+watch_chromium.org, yusukes+watch_chromium.org, derat+watch_chromium.org, gideonwald Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionalternate ntp: add "Recent Tabs" submenu to wrench menu
this is a first-draft implementation of the "Recent tabs" submenu on cros/win,
initial cl was created by dhollowa@ who populated menu w/ non-executable
multiple recently-closed local tabs.
- for local device, show "Reopen closed tab"
* it's enabled if last closed tab can be restored , else it's disabled
- for other devices, show tabs in all windows of all sessions
* sessions and tabs of each session are ranked by recency
* each session has a section header and separator
* selecting a tab menu item restores tab
* if tab has been loaded in history, its favicon is shown
* max 3 sessions, most-recent session first
* max 4 tabs per session, most recent tabs independent of what window they
were from, reverse chronological and no window delineation
- if there's no device, show disabled "No tabs from other devices"
- added unittest, which populates SessionModelAssociator with fake foreign sessions via protocol buffer
BUG=159015
TEST=verify per bug rpt
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167362
Patch Set 1 #
Total comments: 45
Patch Set 2 : addressed scott's and cole's comments, rebased #Patch Set 3 : addressed scott's comments, code cleanup #Patch Set 4 : fixed build errors from tryjobs #Patch Set 5 : fixed accel key for reopen tab - it broke unittest #
Total comments: 8
Patch Set 6 : addressed scott's and cole's comments, add unittest #Patch Set 7 : add comment, fix test #Patch Set 8 : fixed disposition, minor style cleanup #
Total comments: 19
Patch Set 9 : addressed scott's and fred's comments #Patch Set 10 : remove unused function #Patch Set 11 : addressed scott's comments #
Total comments: 14
Patch Set 12 : addressed fred's comments #
Total comments: 4
Patch Set 13 : addressed fred's comments #
Total comments: 8
Patch Set 14 : addressed scott's comments #Patch Set 15 : addressed scott's comments #Patch Set 16 : rebased to resolve conflicts #Messages
Total messages: 40 (0 generated)
sky: pls look at everything. jeremy, pls look at recent_tabs_menu_model.*. thx.
https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/browser_com... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/browser_com... chrome/browser/ui/browser_command_controller.cc:855: !Profile::IsGuestSession()); I don't think we should show it for incognito windows either. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... File chrome/browser/ui/toolbar/recent_tabs_menu_model_delegate.h (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_menu_model_delegate.h:33: views::MenuItemView* menu_item_; You shouldn't have views specific code in this directory. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:63: struct RecentTabsSubMenuModel::NavigationItem { Add description. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:66: NavigationItem(const std::string& session_tag, nit: newline between 65/66 and 70/71. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:69: : session_tag(session_tag), tab_id(tab_id), url(url) { since you wrapped the constructor wrap each param onto its own line. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:178: #if !defined(SHOW_MULTIPLE_RECENTLY_CLOSED_TABS) Why the ifdef? https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:183: TabRestoreServiceFactory::GetForProfile(browser_->profile()); What happens if the contents of the tabrestoreservice change after you've built your model? https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:188: std::set<NavigationItem> seen_tabs; Is there a particular reason for trying to avoid closed tabs like this? https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:201: std::vector<TabRestoreService::Tab>::const_iterator it; Use a different name thant that of it defined on 189. Or better yet move to a separate function. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:204: if (BuildRecentlyClosedTabItem(tab, &seen_tabs)) Why are you extracting the tabs from window entries? We don't do this on the ntp now. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:209: if (!tab) If this were NULL you would have crashed on 193. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:230: return false; nit: indentation is off. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:284: FaviconService::FaviconForURLParams(browser_->profile(), url, nit: indentation is off here. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:30: class RecentTabsSubMenuModel : public ui::SimpleMenuModel, Add description, and I think this should be RecentlyClosedTabsSubMenuModel. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar_view.cc:425: wrench_menu_model_.reset(new WrenchMenuModel(this, browser_)); This order goes against your comment in the header. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/wrenc... File chrome/browser/ui/views/wrench_menu.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/wrenc... chrome/browser/ui/views/wrench_menu.cc:762: recent_tabs_menu_model_delegate_.reset(); Is there a reason to explicitly destroy this here?
i've addressed scott's and cole's comments in patch set 2. regarding cole's comments, they're: - disable device section header (so that it won't be highlighted when hovered over) - limit menu item text to 320px - change "Reopen closed tab" to "Reopen closed window" when it's of window type. ptal. thx. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/browser_com... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/browser_com... chrome/browser/ui/browser_command_controller.cc:855: !Profile::IsGuestSession()); On 2012/11/07 00:10:47, sky wrote: > I don't think we should show it for incognito windows either. IDC_RECENT_TABS_MENU is only added if instant extended api is enabled, and that's disabled for incognito. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... File chrome/browser/ui/toolbar/recent_tabs_menu_model_delegate.h (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_menu_model_delegate.h:33: views::MenuItemView* menu_item_; On 2012/11/07 00:10:47, sky wrote: > You shouldn't have views specific code in this directory. Done. i initially had this as a private class of WrenchMenu, then i got greedy and thot i cld use it for all platforms :( i've moved it back to a private class of WrenchMenu. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:63: struct RecentTabsSubMenuModel::NavigationItem { On 2012/11/07 00:10:47, sky wrote: > Add description. Done. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:66: NavigationItem(const std::string& session_tag, On 2012/11/07 00:10:47, sky wrote: > nit: newline between 65/66 and 70/71. Done. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:69: : session_tag(session_tag), tab_id(tab_id), url(url) { On 2012/11/07 00:10:47, sky wrote: > since you wrapped the constructor wrap each param onto its own line. Done. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:178: #if !defined(SHOW_MULTIPLE_RECENTLY_CLOSED_TABS) On 2012/11/07 00:10:47, sky wrote: > Why the ifdef? i explained it in the cl description. david wrote BuildRecentlyClosed to populate menu with multiple closed tabs. a few days later, cole said to only have the last closed tab, but dogfood might dictate we allow multiple closed tabs later. given that this is a new file with no revisions to revert to, and i didn't want to waste the effort that david and i have spent on multiple closed tabs, i block that code with ifdef. so, for BuildRecentlyClosed, only the the first 2 lines will execute for now. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:183: TabRestoreServiceFactory::GetForProfile(browser_->profile()); On 2012/11/07 00:10:47, sky wrote: > What happens if the contents of the tabrestoreservice change after you've built > your model? since this code is not being used for now, i've added a TODO for later. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:188: std::set<NavigationItem> seen_tabs; On 2012/11/07 00:10:47, sky wrote: > Is there a particular reason for trying to avoid closed tabs like this? i believe david didn't want to repeat tabs with the same url. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:201: std::vector<TabRestoreService::Tab>::const_iterator it; On 2012/11/07 00:10:47, sky wrote: > Use a different name thant that of it defined on 189. Or better yet move to a > separate function. Done. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:204: if (BuildRecentlyClosedTabItem(tab, &seen_tabs)) On 2012/11/07 00:10:47, sky wrote: > Why are you extracting the tabs from window entries? We don't do this on the ntp > now. i assume u're talking about RecentClosedTabsHandler.cc::CreateRecentlyClosedValues. when entry type is TabRestoreService::WINDOW, it calls WindowToValue which calls TabToValue for each tab in the window. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:209: if (!tab) On 2012/11/07 00:10:47, sky wrote: > If this were NULL you would have crashed on 193. hm.. i thot that even if entry is !NULL, if it's type is not TabRestoreService::Tab, tab wld still be NULL? https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:230: return false; On 2012/11/07 00:10:47, sky wrote: > nit: indentation is off. Done. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:284: FaviconService::FaviconForURLParams(browser_->profile(), url, On 2012/11/07 00:10:47, sky wrote: > nit: indentation is off here. Done. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:30: class RecentTabsSubMenuModel : public ui::SimpleMenuModel, On 2012/11/07 00:10:47, sky wrote: > Add description, and I think this should be RecentlyClosedTabsSubMenuModel. added description. but didn't rename, 'cos it contains the last closed tab of local device and open tabs of other devices. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar_view.cc:425: wrench_menu_model_.reset(new WrenchMenuModel(this, browser_)); On 2012/11/07 00:10:47, sky wrote: > This order goes against your comment in the header. my comment in header said wrench_menu_model_ shld be listed later so that it's destructed earlier. so here in construction, shldn't it be constructed earlier? https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/wrenc... File chrome/browser/ui/views/wrench_menu.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/wrenc... chrome/browser/ui/views/wrench_menu.cc:762: recent_tabs_menu_model_delegate_.reset(); On 2012/11/07 00:10:47, sky wrote: > Is there a reason to explicitly destroy this here? the only reason is wherever we stop observing bookmark model, i do the same thing for recent tabs by resetting the delegate. is this wrong?
https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/browser_com... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/browser_com... chrome/browser/ui/browser_command_controller.cc:855: !Profile::IsGuestSession()); On 2012/11/07 02:29:46, kuan wrote: > On 2012/11/07 00:10:47, sky wrote: > > I don't think we should show it for incognito windows either. > > IDC_RECENT_TABS_MENU is only added if instant extended api is enabled, and > that's disabled for incognito. Even so, shouldn't we correctly disable the command? https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:178: #if !defined(SHOW_MULTIPLE_RECENTLY_CLOSED_TABS) On 2012/11/07 02:29:46, kuan wrote: > On 2012/11/07 00:10:47, sky wrote: > > Why the ifdef? > > i explained it in the cl description. > david wrote BuildRecentlyClosed to populate menu with multiple closed tabs. > a few days later, cole said to only have the last closed tab, but dogfood might > dictate we allow multiple closed tabs later. > given that this is a new file with no revisions to revert to, and i didn't want > to waste the effort that david and i have spent on multiple closed tabs, i block > that code with ifdef. > so, for BuildRecentlyClosed, only the the first 2 lines will execute for now. The ifdefs make the code a lot harder to follow. Additionally we generally don't land things we 'might' want unless we know we want to try it and they are behind a flag. That isn't the case here. Think of what the code would look like if we left in all variations we've tried. Oy! Keep the code at what we're going with now. For the !defined(SHOW_MULTIPLE_RECENTLY_CLOSED_TABS), how do you know there is at least one tab to restore? https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:183: TabRestoreServiceFactory::GetForProfile(browser_->profile()); On 2012/11/07 02:29:46, kuan wrote: > On 2012/11/07 00:10:47, sky wrote: > > What happens if the contents of the tabrestoreservice change after you've > built > > your model? > > since this code is not being used for now, i've added a TODO for later. Yet more of a reason to nuke this code. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:209: if (!tab) On 2012/11/07 02:29:46, kuan wrote: > On 2012/11/07 00:10:47, sky wrote: > > If this were NULL you would have crashed on 193. > > hm.. i thot that even if entry is !NULL, if it's type is not > TabRestoreService::Tab, tab wld still be NULL? No, you would basically be casting to the wrong thing. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/wrenc... File chrome/browser/ui/views/wrench_menu.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/wrenc... chrome/browser/ui/views/wrench_menu.cc:762: recent_tabs_menu_model_delegate_.reset(); On 2012/11/07 02:29:46, kuan wrote: > On 2012/11/07 00:10:47, sky wrote: > > Is there a reason to explicitly destroy this here? > > the only reason is wherever we stop observing bookmark model, i do the same > thing for recent tabs by resetting the delegate. is this wrong? But your code doesn't need to be cleaned up explicitly right? Running the destructor and destroying the scoped_ptr should do the right thing.
i've addressed scott's comments in patch set 3, plus some code cleanup. ptal. thx. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/browser_com... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/browser_com... chrome/browser/ui/browser_command_controller.cc:855: !Profile::IsGuestSession()); On 2012/11/07 14:40:12, sky wrote: > On 2012/11/07 02:29:46, kuan wrote: > > On 2012/11/07 00:10:47, sky wrote: > > > I don't think we should show it for incognito windows either. > > > > IDC_RECENT_TABS_MENU is only added if instant extended api is enabled, and > > that's disabled for incognito. > > Even so, shouldn't we correctly disable the command? Done. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:178: #if !defined(SHOW_MULTIPLE_RECENTLY_CLOSED_TABS) On 2012/11/07 14:40:12, sky wrote: > On 2012/11/07 02:29:46, kuan wrote: > > On 2012/11/07 00:10:47, sky wrote: > > > Why the ifdef? > > > > i explained it in the cl description. > > david wrote BuildRecentlyClosed to populate menu with multiple closed tabs. > > a few days later, cole said to only have the last closed tab, but dogfood > might > > dictate we allow multiple closed tabs later. > > given that this is a new file with no revisions to revert to, and i didn't > want > > to waste the effort that david and i have spent on multiple closed tabs, i > block > > that code with ifdef. > > so, for BuildRecentlyClosed, only the the first 2 lines will execute for now. > > The ifdefs make the code a lot harder to follow. Additionally we generally don't > land things we 'might' want unless we know we want to try it and they are behind > a flag. That isn't the case here. Think of what the code would look like if we > left in all variations we've tried. Oy! Keep the code at what we're going with > now. > > For the !defined(SHOW_MULTIPLE_RECENTLY_CLOSED_TABS), how do you know there is > at least one tab to restore? Done. "Reopen closed tab" is disabled if there's no tab to restore; this is already handled in the current chrome, 'cos it's already a menu item when u right-click in the non-client area right of tabstrip. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:183: TabRestoreServiceFactory::GetForProfile(browser_->profile()); On 2012/11/07 14:40:12, sky wrote: > On 2012/11/07 02:29:46, kuan wrote: > > On 2012/11/07 00:10:47, sky wrote: > > > What happens if the contents of the tabrestoreservice change after you've > > built > > > your model? > > > > since this code is not being used for now, i've added a TODO for later. > > Yet more of a reason to nuke this code. Done. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:209: if (!tab) On 2012/11/07 14:40:12, sky wrote: > On 2012/11/07 02:29:46, kuan wrote: > > On 2012/11/07 00:10:47, sky wrote: > > > If this were NULL you would have crashed on 193. > > > > hm.. i thot that even if entry is !NULL, if it's type is not > > TabRestoreService::Tab, tab wld still be NULL? > > No, you would basically be casting to the wrong thing. Done. code is nuked. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar_view.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar_view.cc:425: wrench_menu_model_.reset(new WrenchMenuModel(this, browser_)); On 2012/11/07 02:29:46, kuan wrote: > On 2012/11/07 00:10:47, sky wrote: > > This order goes against your comment in the header. > > my comment in header said wrench_menu_model_ shld be listed later so that it's > destructed earlier. so here in construction, shldn't it be constructed earlier? now, i understand what u mean; u're right, i shld construct it later. i forgot that scoped_ptr::reset will destruct the previous pointer b4 constructing new one, so i shld reset model first. https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/wrenc... File chrome/browser/ui/views/wrench_menu.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/views/wrenc... chrome/browser/ui/views/wrench_menu.cc:762: recent_tabs_menu_model_delegate_.reset(); On 2012/11/07 14:40:12, sky wrote: > On 2012/11/07 02:29:46, kuan wrote: > > On 2012/11/07 00:10:47, sky wrote: > > > Is there a reason to explicitly destroy this here? > > > > the only reason is wherever we stop observing bookmark model, i do the same > > thing for recent tabs by resetting the delegate. is this wrong? > > But your code doesn't need to be cleaned up explicitly right? Running the > destructor and destroying the scoped_ptr should do the right thing. Done.
https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:204: if (BuildRecentlyClosedTabItem(tab, &seen_tabs)) On 2012/11/07 02:29:46, kuan wrote: > On 2012/11/07 00:10:47, sky wrote: > > Why are you extracting the tabs from window entries? We don't do this on the > ntp > > now. > > i assume u're talking about > RecentClosedTabsHandler.cc::CreateRecentlyClosedValues. > when entry type is TabRestoreService::WINDOW, it calls WindowToValue which calls > TabToValue for each tab in the window. I'm referring to why do you look through all the tabs in a window adding an entry to the model for each tab. Why don't we have a single entry for the window? This what the current ntp does as well as restoring a closed window.
https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:204: if (BuildRecentlyClosedTabItem(tab, &seen_tabs)) On 2012/11/07 17:35:02, sky wrote: > On 2012/11/07 02:29:46, kuan wrote: > > On 2012/11/07 00:10:47, sky wrote: > > > Why are you extracting the tabs from window entries? We don't do this on the > > ntp > > > now. > > > > i assume u're talking about > > RecentClosedTabsHandler.cc::CreateRecentlyClosedValues. > > when entry type is TabRestoreService::WINDOW, it calls WindowToValue which > calls > > TabToValue for each tab in the window. > > I'm referring to why do you look through all the tabs in a window adding an > entry to the model for each tab. Why don't we have a single entry for the > window? This what the current ntp does as well as restoring a closed window. this code is nuked in patch set 3.
On Wed, Nov 7, 2012 at 9:38 AM, <kuan@chromium.org> wrote: > > https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... > File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): > > https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:204: if > (BuildRecentlyClosedTabItem(tab, &seen_tabs)) > On 2012/11/07 17:35:02, sky wrote: >> >> On 2012/11/07 02:29:46, kuan wrote: >> > On 2012/11/07 00:10:47, sky wrote: >> > > Why are you extracting the tabs from window entries? We don't do > > this on the >> >> > ntp >> > > now. >> > >> > i assume u're talking about >> > RecentClosedTabsHandler.cc::CreateRecentlyClosedValues. >> > when entry type is TabRestoreService::WINDOW, it calls WindowToValue > > which >> >> calls >> > TabToValue for each tab in the window. > > >> I'm referring to why do you look through all the tabs in a window > > adding an >> >> entry to the model for each tab. Why don't we have a single entry for > > the >> >> window? This what the current ntp does as well as restoring a closed > > window. > > this code is nuked in patch set 3. > > https://codereview.chromium.org/11298004/ Aren't you still doing it in RecentTabsSubMenuModel::BuildDevices() ? -Scott
On 2012/11/07 17:41:52, sky wrote: > On Wed, Nov 7, 2012 at 9:38 AM, <mailto:kuan@chromium.org> wrote: > > > > > https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... > > File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): > > > > > https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... > > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:204: if > > (BuildRecentlyClosedTabItem(tab, &seen_tabs)) > > On 2012/11/07 17:35:02, sky wrote: > >> > >> On 2012/11/07 02:29:46, kuan wrote: > >> > On 2012/11/07 00:10:47, sky wrote: > >> > > Why are you extracting the tabs from window entries? We don't do > > > > this on the > >> > >> > ntp > >> > > now. > >> > > >> > i assume u're talking about > >> > RecentClosedTabsHandler.cc::CreateRecentlyClosedValues. > >> > when entry type is TabRestoreService::WINDOW, it calls WindowToValue > > > > which > >> > >> calls > >> > TabToValue for each tab in the window. > > > > > >> I'm referring to why do you look through all the tabs in a window > > > > adding an > >> > >> entry to the model for each tab. Why don't we have a single entry for > > > > the > >> > >> window? This what the current ntp does as well as restoring a closed > > > > window. > > > > this code is nuked in patch set 3. > > > > https://codereview.chromium.org/11298004/ > > Aren't you still doing it in RecentTabsSubMenuModel::BuildDevices() ? > > -Scott for our ntp, we want a flat layout with tabs of each window.
fyi, patches set 4 and 5 are fixes for errors from tryjobs, especially #5 changes the way i get accelerator key for reopening last tab 'cos my previous way broke chromeos unittest.
How come? Doesn't that mean if you close a single window with lots of tabs you effectively don't see all the tabs in that window? -Scott On Wed, Nov 7, 2012 at 9:45 AM, <kuan@chromium.org> wrote: > On 2012/11/07 17:41:52, sky wrote: > >> On Wed, Nov 7, 2012 at 9:38 AM, <mailto:kuan@chromium.org> wrote: >> > >> > > > > https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... >> >> > File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): >> > >> > > > > https://codereview.chromium.org/11298004/diff/1/chrome/browser/ui/toolbar/rec... >> >> > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:204: if >> > (BuildRecentlyClosedTabItem(tab, &seen_tabs)) >> > On 2012/11/07 17:35:02, sky wrote: >> >> >> >> On 2012/11/07 02:29:46, kuan wrote: >> >> > On 2012/11/07 00:10:47, sky wrote: >> >> > > Why are you extracting the tabs from window entries? We don't do >> > >> > this on the >> >> >> >> > ntp >> >> > > now. >> >> > >> >> > i assume u're talking about >> >> > RecentClosedTabsHandler.cc::CreateRecentlyClosedValues. >> >> > when entry type is TabRestoreService::WINDOW, it calls WindowToValue >> > >> > which >> >> >> >> calls >> >> > TabToValue for each tab in the window. >> > >> > >> >> I'm referring to why do you look through all the tabs in a window >> > >> > adding an >> >> >> >> entry to the model for each tab. Why don't we have a single entry for >> > >> > the >> >> >> >> window? This what the current ntp does as well as restoring a closed >> > >> > window. >> > >> > this code is nuked in patch set 3. >> > >> > https://codereview.chromium.org/11298004/ > > >> Aren't you still doing it in RecentTabsSubMenuModel::BuildDevices() ? > > >> -Scott > > > for our ntp, we want a flat layout with tabs of each window. > > https://codereview.chromium.org/11298004/
Can you create some tests too? https://codereview.chromium.org/11298004/diff/14002/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/14002/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:131: weak_ptr_factory_.InvalidateWeakPtrs(); Isn't this done automatically for you? https://codereview.chromium.org/11298004/diff/14002/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:271: const int kMaxTabsPerSessionToShow = 18; Does this mean we might show 180 items?!? https://codereview.chromium.org/11298004/diff/14002/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right): https://codereview.chromium.org/11298004/diff/14002/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:66: browser_sync::SessionModelAssociator* GetModelAssociator() const; Remove the const since you're returning a non-const object. https://codereview.chromium.org/11298004/diff/14002/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:81: CancelableRequestConsumerTSimple<int> favicon_consumer_; nit: two spaces here.
i've addressed all comments in patch set 6; #7 is just minor cleanup. i've also added unittest. as for restoring a window with many tabs, we won't allow that via menu now. cole may have a future plan to add a menu item at the end to point to history; he said that other devices and history will merge later. ptal. thx. https://codereview.chromium.org/11298004/diff/14002/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/14002/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:131: weak_ptr_factory_.InvalidateWeakPtrs(); On 2012/11/07 22:33:40, sky wrote: > Isn't this done automatically for you? Done. https://codereview.chromium.org/11298004/diff/14002/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:271: const int kMaxTabsPerSessionToShow = 18; On 2012/11/07 22:33:40, sky wrote: > Does this mean we might show 180 items?!? i've changed as per spec from cole who spoke w/ glen, quoting from email from cole: 3 devices, 4 items from each, the most recent tabs independent of what window they were from (reverse chronological and no window delineation). https://codereview.chromium.org/11298004/diff/14002/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right): https://codereview.chromium.org/11298004/diff/14002/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:66: browser_sync::SessionModelAssociator* GetModelAssociator() const; On 2012/11/07 22:33:40, sky wrote: > Remove the const since you're returning a non-const object. Done. https://codereview.chromium.org/11298004/diff/14002/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:81: CancelableRequestConsumerTSimple<int> favicon_consumer_; On 2012/11/07 22:33:40, sky wrote: > nit: two spaces here. Done.
fy, in patch set 8, i fixed disposition to restore foreign tab in a new tab and activate it, plus minor code moving in unittest. fred, cld u pls look at session_model_associator.h? i added a friend class for my unittest to populate foreign sessions via syncd_session_tracker_ and PopulateSessionWindowFromSpecifics. thx.
https://codereview.chromium.org/11298004/diff/14005/chrome/browser/sync/glue/... File chrome/browser/sync/glue/session_model_associator.h (right): https://codereview.chromium.org/11298004/diff/14005/chrome/browser/sync/glue/... chrome/browser/sync/glue/session_model_associator.h:238: friend class SessionModelAssociatorForRecentTabsSubMenuModelTest; I'd prefer to instead expose a method like "Tracker* GetTrackerForTest()" instead. Can you do that instead?
https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:173: if (service && Check the size here. https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:193: DCHECK(!model_.empty() && command_id > 0); How about starting at a particular command id for the ones you care about. That seems less error prone than assuming you add empty items to the model. https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:268: for (size_t i = 0; i < std::min(sessions.size(), kMaxSessionsToShow); ++i) { Shouldn't you only consider sessions you actually add something for? https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:298: !k ? session->session_name : std::string(), need_separator)) { Since BuildForeightTabItem may return false, your !k check here isn't quite right. You really want 'have I added a type for this session yet'. I think this would be less error prone if you pruned out tabs you know you won't add around 283. That way BuildForeignTabItem doesn't need a return value and then !k here is right. https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:323: model_.push_back(NavigationItem()); Why do you need to add an item to the model here and 328? https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:345: default_favicon_ = &rb.GetNativeImageNamed(IDR_DEFAULT_FAVICON); Is there a compelling reason to make this a pointer instead of a value? https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:354: FaviconService::Handle handle = favicon_service->GetFaviconImageForURL( See email from Kai about this being deprecated.
https://codereview.chromium.org/11298004/diff/14005/chrome/browser/sync/glue/... File chrome/browser/sync/glue/session_model_associator.h (right): https://codereview.chromium.org/11298004/diff/14005/chrome/browser/sync/glue/... chrome/browser/sync/glue/session_model_associator.h:238: friend class SessionModelAssociatorForRecentTabsSubMenuModelTest; On 2012/11/09 18:11:58, akalin wrote: > I'd prefer to instead expose a method like "Tracker* GetTrackerForTest()" > instead. Can you do that instead? i also need PopulateSessionWindowFromSpecifics. so providing GetTrackerForTest won't be enough.
On 2012/11/09 18:17:17, kuan wrote: > https://codereview.chromium.org/11298004/diff/14005/chrome/browser/sync/glue/... > File chrome/browser/sync/glue/session_model_associator.h (right): > > https://codereview.chromium.org/11298004/diff/14005/chrome/browser/sync/glue/... > chrome/browser/sync/glue/session_model_associator.h:238: friend class > SessionModelAssociatorForRecentTabsSubMenuModelTest; > On 2012/11/09 18:11:58, akalin wrote: > > I'd prefer to instead expose a method like "Tracker* GetTrackerForTest()" > > instead. Can you do that instead? > > i also need PopulateSessionWindowFromSpecifics. so providing GetTrackerForTest > won't be enough. Perhaps PopulateSessionWindowFromSpecificsForTest(), then? I'm just wary about providing friend access to a class that's "far away" from the file. :/
i've addressed scott's and fred's comments in patch set 9; #10 is just removal of 1 unused function. ptal. thx. https://codereview.chromium.org/11298004/diff/14005/chrome/browser/sync/glue/... File chrome/browser/sync/glue/session_model_associator.h (right): https://codereview.chromium.org/11298004/diff/14005/chrome/browser/sync/glue/... chrome/browser/sync/glue/session_model_associator.h:238: friend class SessionModelAssociatorForRecentTabsSubMenuModelTest; On 2012/11/09 18:17:17, kuan wrote: > On 2012/11/09 18:11:58, akalin wrote: > > I'd prefer to instead expose a method like "Tracker* GetTrackerForTest()" > > instead. Can you do that instead? > > i also need PopulateSessionWindowFromSpecifics. so providing GetTrackerForTest > won't be enough. Done. i've enclosed the functions within #ifdef UNIT_TEST. https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:173: if (service && On 2012/11/09 18:13:05, sky wrote: > Check the size here. size is checked in CanRestoreTab which is called from IsCommandIdEnabled @170. https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:193: DCHECK(!model_.empty() && command_id > 0); On 2012/11/09 18:13:05, sky wrote: > How about starting at a particular command id for the ones you care about. That > seems less error prone than assuming you add empty items to the model. Done. https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:268: for (size_t i = 0; i < std::min(sessions.size(), kMaxSessionsToShow); ++i) { On 2012/11/09 18:13:05, sky wrote: > Shouldn't you only consider sessions you actually add something for? Done. https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:298: !k ? session->session_name : std::string(), need_separator)) { On 2012/11/09 18:13:05, sky wrote: > Since BuildForeightTabItem may return false, your !k check here isn't quite > right. You really want 'have I added a type for this session yet'. > > I think this would be less error prone if you pruned out tabs you know you won't > add around 283. That way BuildForeignTabItem doesn't need a return value and > then !k here is right. duh.. i had previously counted the tabs added, then i removed it carelessly :( i've now fixed it by counting tabs added. i can't prune out tabs at @283 'cos i don't know which one is most recent. https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:323: model_.push_back(NavigationItem()); On 2012/11/09 18:13:05, sky wrote: > Why do you need to add an item to the model here and 328? Done. https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:345: default_favicon_ = &rb.GetNativeImageNamed(IDR_DEFAULT_FAVICON); On 2012/11/09 18:13:05, sky wrote: > Is there a compelling reason to make this a pointer instead of a value? Done. https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:354: FaviconService::Handle handle = favicon_service->GetFaviconImageForURL( On 2012/11/09 18:13:05, sky wrote: > See email from Kai about this being deprecated. i just saw that; however he hasn't changed FaviconService::FaviconForURLParams which is i use here. so i don't think i can change it yet. if he changes that b4 i land my cl, i will of course have to change, since it won't build anyway.
https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:298: !k ? session->session_name : std::string(), need_separator)) { On 2012/11/09 20:27:52, kuan wrote: > On 2012/11/09 18:13:05, sky wrote: > > Since BuildForeightTabItem may return false, your !k check here isn't quite > > right. You really want 'have I added a type for this session yet'. > > > > I think this would be less error prone if you pruned out tabs you know you > won't > > add around 283. That way BuildForeignTabItem doesn't need a return value and > > then !k here is right. > > duh.. i had previously counted the tabs added, then i removed it carelessly :( > i've now fixed it by counting tabs added. i can't prune out tabs at @283 'cos i > don't know which one is most recent. By prune I meant remove ones that match the criteria in 312-319 (no navigations or on the new tab). That way you're only left with valid tabs.
i've addressed all comment in patch set 11. ptal. thx. https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/14005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:298: !k ? session->session_name : std::string(), need_separator)) { On 2012/11/09 21:44:56, sky wrote: > On 2012/11/09 20:27:52, kuan wrote: > > On 2012/11/09 18:13:05, sky wrote: > > > Since BuildForeightTabItem may return false, your !k check here isn't quite > > > right. You really want 'have I added a type for this session yet'. > > > > > > I think this would be less error prone if you pruned out tabs you know you > > won't > > > add around 283. That way BuildForeignTabItem doesn't need a return value and > > > then !k here is right. > > > > duh.. i had previously counted the tabs added, then i removed it carelessly :( > > i've now fixed it by counting tabs added. i can't prune out tabs at @283 'cos > i > > don't know which one is most recent. > > By prune I meant remove ones that match the criteria in 312-319 (no navigations > or on the new tab). That way you're only left with valid tabs. Done.
https://codereview.chromium.org/11298004/diff/12012/chrome/browser/sync/glue/... File chrome/browser/sync/glue/session_model_associator.h (right): https://codereview.chromium.org/11298004/diff/12012/chrome/browser/sync/glue/... chrome/browser/sync/glue/session_model_associator.h:236: #if defined(UNIT_TEST) i don't think we should use this ifdef. having different definitions per compilation unit is technically undefined behavior, and the inline functions should get optimized out anyway for non-unit-test executables. https://codereview.chromium.org/11298004/diff/12012/chrome/browser/sync/glue/... chrome/browser/sync/glue/session_model_associator.h:236: #if defined(UNIT_TEST) But see comments in unit test file -- I think you can do what you want while just working with the public interface. https://codereview.chromium.org/11298004/diff/12012/chrome/browser/sync/glue/... chrome/browser/sync/glue/session_model_associator.h:238: SyncedSessionTracker& GetSyncedSessionTrackerForTest() { non-const refs are against style guide. please use a pointer. https://codereview.chromium.org/11298004/diff/12012/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/11298004/diff/12012/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:136: const sync_pb::SessionWindow& window_pb) { actually, can't you just use the public function AssociateForeignSpecifics()? You'll have to build a SessionSpecifics object yourself, but that's not too bad. https://codereview.chromium.org/11298004/diff/12012/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:151: browser_sync::SyncedSession* session = GetSession(session_id); can you do this using public functions? Maybe use AssociateForeignSpecifics again? https://codereview.chromium.org/11298004/diff/12012/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:240: ASSERT_EQ(3U, associator.tracker().num_synced_tabs(ToSessionTag(session_id))); can you get the num synced tabs using GetForeignSession()? https://codereview.chromium.org/11298004/diff/12012/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:262: ASSERT_EQ(2U, associator.tracker().num_synced_sessions()); GetAllForeignSessions()?
https://codereview.chromium.org/11298004/diff/12012/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/11298004/diff/12012/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:136: const sync_pb::SessionWindow& window_pb) { On 2012/11/10 00:08:40, akalin wrote: > actually, can't you just use the public function AssociateForeignSpecifics()? > You'll have to build a SessionSpecifics object yourself, but that's not too bad. i tried implementing building the SessionSpecifics object for session, window and tabs. unfortunately, AssociateSessionSpecifics failed 'cos there's no machine tag. apparently, it's more than just populating these data. :( do i have to do exactly what ProfileSyncedServiceSessionTest does? it creates a proper TestProfileSyncedService, profile, io thread, etc. i don't know the inner workings of ProfileSyncedServiceSession and SessionModelAssociator to know what need to be initialized b4 foreign sessions can be added, e.g. if io thread is needed. won't duplicating the entire ProfileSyncedServiceSessionTest be an overkill for my test? i'm not testing synced service, i'm testing that the menu shows whatever the foreign sessions and their tabs are. especially, when i've added the access methods in SessionModelAssociator only for #ifdef UNIT_TEST.
> i tried implementing building the SessionSpecifics object for session, window > and tabs. unfortunately, AssociateSessionSpecifics failed 'cos there's no > machine tag. apparently, it's more than just populating these data. :( If that's the only problem, then that's easy enough to fix -- just add a SetCurrentMachineTagForTest() setter. > do i have to do exactly what ProfileSyncedServiceSessionTest does? it creates a > proper TestProfileSyncedService, profile, io thread, etc. i don't know the > inner workings of ProfileSyncedServiceSession and SessionModelAssociator to know > what need to be initialized b4 foreign sessions can be added, e.g. if io thread > is needed. > > won't duplicating the entire ProfileSyncedServiceSessionTest be an overkill for > my test? i'm not testing synced service, i'm testing that the menu shows > whatever the foreign sessions and their tabs are. especially, when i've added > the access methods in SessionModelAssociator only for #ifdef UNIT_TEST. That shouldn't be necessary.
i've addressed all comments in patch set 12. ptal. thx. https://codereview.chromium.org/11298004/diff/12012/chrome/browser/sync/glue/... File chrome/browser/sync/glue/session_model_associator.h (right): https://codereview.chromium.org/11298004/diff/12012/chrome/browser/sync/glue/... chrome/browser/sync/glue/session_model_associator.h:236: #if defined(UNIT_TEST) On 2012/11/10 00:08:40, akalin wrote: > But see comments in unit test file -- I think you can do what you want while > just working with the public interface. i assume u mean i can keep it? https://codereview.chromium.org/11298004/diff/12012/chrome/browser/sync/glue/... chrome/browser/sync/glue/session_model_associator.h:238: SyncedSessionTracker& GetSyncedSessionTrackerForTest() { On 2012/11/10 00:08:40, akalin wrote: > non-const refs are against style guide. please use a pointer. function is removed. https://codereview.chromium.org/11298004/diff/12012/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/11298004/diff/12012/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:136: const sync_pb::SessionWindow& window_pb) { On 2012/11/10 17:07:04, kuan wrote: > On 2012/11/10 00:08:40, akalin wrote: > > actually, can't you just use the public function AssociateForeignSpecifics()? > > You'll have to build a SessionSpecifics object yourself, but that's not too > bad. > > i tried implementing building the SessionSpecifics object for session, window > and tabs. unfortunately, AssociateSessionSpecifics failed 'cos there's no > machine tag. apparently, it's more than just populating these data. :( > > do i have to do exactly what ProfileSyncedServiceSessionTest does? it creates a > proper TestProfileSyncedService, profile, io thread, etc. i don't know the > inner workings of ProfileSyncedServiceSession and SessionModelAssociator to know > what need to be initialized b4 foreign sessions can be added, e.g. if io thread > is needed. > > won't duplicating the entire ProfileSyncedServiceSessionTest be an overkill for > my test? i'm not testing synced service, i'm testing that the menu shows > whatever the foreign sessions and their tabs are. especially, when i've added > the access methods in SessionModelAssociator only for #ifdef UNIT_TEST. Done. https://codereview.chromium.org/11298004/diff/12012/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:151: browser_sync::SyncedSession* session = GetSession(session_id); On 2012/11/10 00:08:40, akalin wrote: > can you do this using public functions? Maybe use AssociateForeignSpecifics > again? Done. https://codereview.chromium.org/11298004/diff/12012/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:240: ASSERT_EQ(3U, associator.tracker().num_synced_tabs(ToSessionTag(session_id))); On 2012/11/10 00:08:40, akalin wrote: > can you get the num synced tabs using GetForeignSession()? Done. https://codereview.chromium.org/11298004/diff/12012/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:262: ASSERT_EQ(2U, associator.tracker().num_synced_sessions()); On 2012/11/10 00:08:40, akalin wrote: > GetAllForeignSessions()? Done.
sync stuff lgtm after nits below thanks for refactoring! https://codereview.chromium.org/11298004/diff/6025/chrome/browser/sync/glue/s... File chrome/browser/sync/glue/session_model_associator.h (right): https://codereview.chromium.org/11298004/diff/6025/chrome/browser/sync/glue/s... chrome/browser/sync/glue/session_model_associator.h:236: #if defined(UNIT_TEST) sorry, i wasn't clear -- i still want the "#if defined(...)" part removed. https://codereview.chromium.org/11298004/diff/6025/chrome/browser/sync/glue/s... chrome/browser/sync/glue/session_model_associator.h:237: // Sets the machine tag for testing. you can remove the comment -- i think the function name is self-explanatory
i've addressed all comments in patch set 12. thx. https://codereview.chromium.org/11298004/diff/6025/chrome/browser/sync/glue/s... File chrome/browser/sync/glue/session_model_associator.h (right): https://codereview.chromium.org/11298004/diff/6025/chrome/browser/sync/glue/s... chrome/browser/sync/glue/session_model_associator.h:236: #if defined(UNIT_TEST) On 2012/11/10 21:47:40, akalin wrote: > sorry, i wasn't clear -- i still want the "#if defined(...)" part removed. Done. https://codereview.chromium.org/11298004/diff/6025/chrome/browser/sync/glue/s... chrome/browser/sync/glue/session_model_associator.h:237: // Sets the machine tag for testing. On 2012/11/10 21:47:40, akalin wrote: > you can remove the comment -- i think the function name is self-explanatory Done.
https://codereview.chromium.org/11298004/diff/3010/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/3010/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:145: DCHECK(setup_for_test); Why the DCHECK for setup_for_test? If always true, why the boolean? https://codereview.chromium.org/11298004/diff/3010/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:186: if (service && !service->entries().empty() https://codereview.chromium.org/11298004/diff/3010/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:347: const int kMaxTabTitleWidth = 320; How come you need this? Does the menu handle it for you? https://codereview.chromium.org/11298004/diff/3010/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:358: if (default_favicon_.IsEmpty()) { nit: move to constructor so you don't need to check each time through.
i've addressed scott's comments in patch set 14. ptal. thx. https://codereview.chromium.org/11298004/diff/3010/chrome/browser/ui/toolbar/... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/11298004/diff/3010/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:145: DCHECK(setup_for_test); On 2012/11/12 04:13:03, sky wrote: > Why the DCHECK for setup_for_test? If always true, why the boolean? Done. i was following what SessionModelAssociator did. https://codereview.chromium.org/11298004/diff/3010/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:186: if (service && On 2012/11/12 04:13:03, sky wrote: > !service->entries().empty() Done. https://codereview.chromium.org/11298004/diff/3010/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:347: const int kMaxTabTitleWidth = 320; On 2012/11/12 04:13:03, sky wrote: > How come you need this? Does the menu handle it for you? i thought this would fix the text and menu item width. apparently, it only fixed the text length, but not the menu item width. while awaiting review for this cl, i started to look into fixing the menu width in the next cl. i think it's via WrenchMenu::GetMaxWidthForMenu, but haven't confirmed it. the change doesn't look trivial, especially for this big cl. so i've removed elide text for now and will handle it in the next cl. https://codereview.chromium.org/11298004/diff/3010/chrome/browser/ui/toolbar/... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:358: if (default_favicon_.IsEmpty()) { On 2012/11/12 04:13:03, sky wrote: > nit: move to constructor so you don't need to check each time through. Done.
LGTM
Sorry, one question. Is it possible to have only one constructor?
On 2012/11/12 15:27:07, sky wrote: > Sorry, one question. Is it possible to have only one constructor? i thought about that, but that would mean initializing data members in a private method Init(), instead of initializing in the initializer list. will that be ok?
The only difference is the accelerator, right? Is there a reason you can't always do it? On Mon, Nov 12, 2012 at 8:02 AM, <kuan@chromium.org> wrote: > On 2012/11/12 15:27:07, sky wrote: >> >> Sorry, one question. Is it possible to have only one constructor? > > > i thought about that, but that would mean initializing data members in a > private > method Init(), instead of initializing in the initializer list. will that > be > ok? > > https://codereview.chromium.org/11298004/
On 2012/11/12 16:25:08, sky wrote: > The only difference is the accelerator, right? Is there a reason you > can't always do it? > > On Mon, Nov 12, 2012 at 8:02 AM, <mailto:kuan@chromium.org> wrote: > > On 2012/11/12 15:27:07, sky wrote: > >> > >> Sorry, one question. Is it possible to have only one constructor? > > > > > > i thought about that, but that would mean initializing data members in a > > private > > method Init(), instead of initializing in the initializer list. will that > > be > > ok? > > > > https://codereview.chromium.org/11298004/ only 1 constructor would mean SessionModelAssociator param to the constructor, but WrenchMenuModel will pass in NULL when it constructs RecentTabsSubMenuModel. is that ok?
i've combined both constructors into 1 in patch set 15. ptal. thx.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kuan@chromium.org/11298004/11046
Failed to apply patch for chrome/chrome_tests.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/chrome_tests.gypi
Hunk #1 FAILED at 2026.
Hunk #2 FAILED at 2684.
2 out of 2 hunks FAILED -- saving rejects to file chrome/chrome_tests.gypi.rej
Patch: chrome/chrome_tests.gypi
Index: chrome/chrome_tests.gypi
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index
ccfb8ff5ca773d1fcfae70d2bc01921879b5dbcd..f1709b97cd20b9e56b3bd1242bcabda705d29e8a
100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -2026,6 +2026,7 @@
'browser/ui/toolbar/toolbar_model_unittest.cc',
'browser/ui/toolbar/test_toolbar_model.cc',
'browser/ui/toolbar/test_toolbar_model.h',
+ 'browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc',
'browser/ui/toolbar/wrench_menu_model_unittest.cc',
'browser/ui/views/accelerator_table_unittest.cc',
'browser/ui/views/accessibility/accessibility_event_router_views_unittest.cc',
@@ -2684,6 +2685,7 @@
'browser/ui/search/search_delegate_unittest.cc',
'browser/ui/tab_contents/tab_contents_iterator_unittest.cc',
'browser/ui/toolbar/toolbar_model_unittest.cc',
+ 'browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc',
'browser/ui/toolbar/wrench_menu_model_unittest.cc',
'browser/ui/webui/ntp/suggestions_combiner_unittest.cc',
'browser/ui/webui/web_dialog_web_contents_delegate_unittest.cc',
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kuan@chromium.org/11298004/13039
Change committed as 167362 |
