|
|
Created:
7 years, 9 months ago by MAD Modified:
7 years, 9 months ago Reviewers:
Alexei Svitkine (slow), sky CC:
chromium-reviews, gideonwald, Ben Goodger (Google), Peter Kasting Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd new option to open history page to show more other devices
BUG=154655
TEST=Make sure that the "More devices..." option is available in the Recent Tabs sub-menu of the wrench menu, but only when needed, i.e., when there are some tabs of some other devices that can not be shown...
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190084
Patch Set 1 : #
Total comments: 4
Patch Set 2 : CR Comments 1. #Patch Set 3 : Updated tests. #
Total comments: 4
Patch Set 4 : Tests comments. #Patch Set 5 : Always show menu. #
Total comments: 4
Patch Set 6 : Removed unneeded comments. #Patch Set 7 : Updated Mac specific tests. #Patch Set 8 : More... #
Total comments: 10
Patch Set 9 : OWNERS comments 1. #Patch Set 10 : OWNERS comments 2. #
Total comments: 3
Patch Set 11 : DCHECK_GT #
Created: 7 years, 9 months ago
Messages
Total messages: 25 (0 generated)
Thanks! BYE MAD...
https://codereview.chromium.org/12605010/diff/5/chrome/browser/ui/toolbar/rec... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/5/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:333: } // for all sessions Nit: Add a blank line after this. https://codereview.chromium.org/12605010/diff/5/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:334: if (show_more || i < sessions.size()) { Shouldn't this be checking |num_sessions_added| rather than |i|? (Since the loop above can skip certain i's).
How about this? Thanks! BYE MAD... https://codereview.chromium.org/12605010/diff/5/chrome/browser/ui/toolbar/rec... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/5/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:333: } // for all sessions On 2013/03/19 13:48:21, Alexei Svitkine wrote: > Nit: Add a blank line after this. Done. https://codereview.chromium.org/12605010/diff/5/chrome/browser/ui/toolbar/rec... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:334: if (show_more || i < sessions.size()) { On 2013/03/19 13:48:21, Alexei Svitkine wrote: > Shouldn't this be checking |num_sessions_added| rather than |i|? (Since the loop > above can skip certain i's). This is what I was doing initially, but I changed it to i since we could have added less tabs than size() yet still reached the end of the list when we skipped some and comparing it to kMaxSessionsToShow doesn't tell us much either since we may have reached the max on the last i. Unless I'm missing something, this seems more robust, I added a comment to explaining, let me know if it's clear enough. Thanks!
Thanks for the comment explaining it, LGTM.
Time for an OWNERS review? Otherwise let me know and I will ask one of the others... Thansk! BYE MAD
Oups... Need to update unit test... New patch coming up...
OK, please take another look (only updated tests) and let me know if you think we should add a more explicit test. I personally think that this covers it correctly... Thanks! BYE MAD
Still LGTM, with a request to expand the test comments. https://codereview.chromium.org/12605010/diff/13002/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/12605010/diff/13002/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:184: // - max sessions is 3, so only 3 most-recent sessions will show Expand this comment to explain why More devices... should be shown in this case. https://codereview.chromium.org/12605010/diff/13002/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:225: // independent of which window they came from Expand this comment to explain why More devices... should be shown in this case.
Thanks! BYE MAD... https://codereview.chromium.org/12605010/diff/13002/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/12605010/diff/13002/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:184: // - max sessions is 3, so only 3 most-recent sessions will show On 2013/03/19 17:09:33, Alexei Svitkine wrote: > Expand this comment to explain why More devices... should be shown in this case. Done. https://codereview.chromium.org/12605010/diff/13002/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:225: // independent of which window they came from On 2013/03/19 17:09:33, Alexei Svitkine wrote: > Expand this comment to explain why More devices... should be shown in this case. Done.
Sorry... Needed to make another change based on UI lead request... PTAL! BYE MAD
LGTM https://codereview.chromium.org/12605010/diff/17005/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/12605010/diff/17005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:188: // - the "More devices" entry since not all possible tabs are shown. Now, remove this comment since you're unconditionally having that item. https://codereview.chromium.org/12605010/diff/17005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:230: // - the "More devices" entry since not all possible tabs are shown. Now, remove this comment since you're unconditionally having that item.
Thanks again... Ben, all yours for OWNERS review now! BYE MAD https://codereview.chromium.org/12605010/diff/17005/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/12605010/diff/17005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:188: // - the "More devices" entry since not all possible tabs are shown. On 2013/03/19 20:11:01, Alexei Svitkine wrote: > Now, remove this comment since you're unconditionally having that item. D'Ho!ne. Meant to remove them, I guess it slipped... Sorry... https://codereview.chromium.org/12605010/diff/17005/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:230: // - the "More devices" entry since not all possible tabs are shown. On 2013/03/19 20:11:01, Alexei Svitkine wrote: > Now, remove this comment since you're unconditionally having that item. Done.
Could Ben be OOO? If so, Peter can you take over the OWNERS review please? Thanks! BYE MAD
On 2013/03/21 14:27:00, MAD wrote: > Could Ben be OOO? > > If so, Peter can you take over the OWNERS review please? I am WebKit sheriff right now, so I potentially have a long lead time on reviews, if you're looking for approval in short order.
Thanks for letting me know Peter... Maybe sky@ can look at it? BYE MAD
https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:152: } else if (command_id == IDC_RECENT_TABS_MORE) { nit: no else for returns (see style guide), just as 149 is not an else. https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:193: } else if (command_id == IDC_RECENT_TABS_MORE) { same comment as 152. https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:196: ui::DispositionFromEventFlags(event_flags)); Does this do something more than just show history? https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:279: size_t i = 0; Why are you moving i outside the loop? https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:332: AddSeparator(ui::NORMAL_SEPARATOR); Is it possible to get here and nothing has been added?
Thanks sky@! Addressed/answered all comments... Anything else? BYE MAD https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:152: } else if (command_id == IDC_RECENT_TABS_MORE) { On 2013/03/22 03:55:56, sky wrote: > nit: no else for returns (see style guide), just as 149 is not an else. Ho, yes, sorry, I keep forgetting about that one... :-( https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:193: } else if (command_id == IDC_RECENT_TABS_MORE) { On 2013/03/22 03:55:56, sky wrote: > same comment as 152. Done. https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:196: ui::DispositionFromEventFlags(event_flags)); On 2013/03/22 03:55:56, sky wrote: > Does this do something more than just show history? No, why? https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:279: size_t i = 0; On 2013/03/22 03:55:56, sky wrote: > Why are you moving i outside the loop? Oups, sorry about that... I should have brought it back as it was... I did that for a previous version of that change... https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:332: AddSeparator(ui::NORMAL_SEPARATOR); On 2013/03/22 03:55:56, sky wrote: > Is it possible to get here and nothing has been added? Added a comment... Thanks!
Made changes based on off line IM discussion... How about this now? Thanks! BYE MAD
LGTM
https://codereview.chromium.org/12605010/diff/51001/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/51001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:335: DCHECK(GetItemCount() > 0); I don't think this is correct, since items are also added outside of BuildDevices(). For example, RecentTabsSubMenuModel::Build() calls AddItem() on the first message item, before calling BuildDevices(). Also, can you use DCHECK_GT()?
Thanks! Will re-CQ now... :-) BYE MAD https://codereview.chromium.org/12605010/diff/51001/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/51001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:335: DCHECK(GetItemCount() > 0); Changed to _GT, but I don't understand why it's not correct. The comment was about the fact that we would add a separator when there are no other items before in the menu... This DCHECK makes sure it won't happen... Right? That's why I didn't use the term devices/tabs in the comment, but items. Because it's still possible (though I couldn't find ways to reproduce it) that none of the devices returned by GetAllForeignSessions would generate at least one tab. I have tried having only devices with NTP, and they are actually don't show up, seems like there is already filtering from within GetAllForeignSessions, so maybe the filtering we are doing here is not necessary????
LGTM https://codereview.chromium.org/12605010/diff/51001/chrome/browser/ui/toolbar... File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/51001/chrome/browser/ui/toolbar... chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:335: DCHECK(GetItemCount() > 0); On 2013/03/22 19:26:53, MAD wrote: > Changed to _GT, but I don't understand why it's not correct. The comment was > about the fact that we would add a separator when there are no other items > before in the menu... This DCHECK makes sure it won't happen... Right? > > That's why I didn't use the term devices/tabs in the comment, but items. Ah, I didn't realise that was the intention of the check. I thought it was there to check if any tabs were added.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/12605010/56001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/12605010/56001
Message was sent while issue was closed.
Change committed as 190084 |