Chromium Code Reviews

Issue 12605010: Add new option to open history page to show more other devices (Closed)

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
Visibility:
Public.

Description

Add 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 #

Unified diffs Side-by-side diffs Stats (+38 lines, -12 lines)
M chrome/app/generated_resources.grd View 1 chunk +4 lines, -0 lines 0 comments
M chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller_unittest.mm View 2 chunks +2 lines, -2 lines 0 comments
M chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc View 4 chunks +15 lines, -2 lines 0 comments
M chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc View 6 chunks +17 lines, -8 lines 0 comments

Messages

Total messages: 25 (0 generated)
MAD
Thanks! BYE MAD...
7 years, 9 months ago (2013-03-19 02:26:51 UTC) #1
Alexei Svitkine (slow)
https://codereview.chromium.org/12605010/diff/5/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/5/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc#newcode333 chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:333: } // for all sessions Nit: Add a blank ...
7 years, 9 months ago (2013-03-19 13:48:21 UTC) #2
MAD
How about this? Thanks! BYE MAD... https://codereview.chromium.org/12605010/diff/5/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/5/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc#newcode333 chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:333: } // for ...
7 years, 9 months ago (2013-03-19 14:26:14 UTC) #3
Alexei Svitkine (slow)
Thanks for the comment explaining it, LGTM.
7 years, 9 months ago (2013-03-19 14:29:31 UTC) #4
MAD
Time for an OWNERS review? Otherwise let me know and I will ask one of ...
7 years, 9 months ago (2013-03-19 14:32:14 UTC) #5
MAD
Oups... Need to update unit test... New patch coming up...
7 years, 9 months ago (2013-03-19 16:51:09 UTC) #6
MAD
OK, please take another look (only updated tests) and let me know if you think ...
7 years, 9 months ago (2013-03-19 17:05:37 UTC) #7
Alexei Svitkine (slow)
Still LGTM, with a request to expand the test comments. https://codereview.chromium.org/12605010/diff/13002/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/12605010/diff/13002/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc#newcode184 ...
7 years, 9 months ago (2013-03-19 17:09:33 UTC) #8
MAD
Thanks! BYE MAD... https://codereview.chromium.org/12605010/diff/13002/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/12605010/diff/13002/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc#newcode184 chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:184: // - max sessions is 3, ...
7 years, 9 months ago (2013-03-19 17:46:47 UTC) #9
MAD
Sorry... Needed to make another change based on UI lead request... PTAL! BYE MAD
7 years, 9 months ago (2013-03-19 20:06:40 UTC) #10
Alexei Svitkine (slow)
LGTM https://codereview.chromium.org/12605010/diff/17005/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): https://codereview.chromium.org/12605010/diff/17005/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc#newcode188 chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:188: // - the "More devices" entry since not ...
7 years, 9 months ago (2013-03-19 20:11:01 UTC) #11
MAD
Thanks again... Ben, all yours for OWNERS review now! BYE MAD https://codereview.chromium.org/12605010/diff/17005/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right): ...
7 years, 9 months ago (2013-03-19 20:19:22 UTC) #12
MAD
Could Ben be OOO? If so, Peter can you take over the OWNERS review please? ...
7 years, 9 months ago (2013-03-21 14:27:00 UTC) #13
Peter Kasting
On 2013/03/21 14:27:00, MAD wrote: > Could Ben be OOO? > > If so, Peter ...
7 years, 9 months ago (2013-03-21 20:05:05 UTC) #14
MAD
Thanks for letting me know Peter... Maybe sky@ can look at it? BYE MAD
7 years, 9 months ago (2013-03-21 20:09:24 UTC) #15
sky
https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc#newcode152 chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:152: } else if (command_id == IDC_RECENT_TABS_MORE) { nit: no ...
7 years, 9 months ago (2013-03-22 03:55:56 UTC) #16
MAD
Thanks sky@! Addressed/answered all comments... Anything else? BYE MAD https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/33001/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc#newcode152 chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:152: ...
7 years, 9 months ago (2013-03-22 14:17:41 UTC) #17
MAD
Made changes based on off line IM discussion... How about this now? Thanks! BYE MAD
7 years, 9 months ago (2013-03-22 19:15:22 UTC) #18
sky
LGTM
7 years, 9 months ago (2013-03-22 19:16:20 UTC) #19
Alexei Svitkine (slow)
https://codereview.chromium.org/12605010/diff/51001/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/51001/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc#newcode335 chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:335: DCHECK(GetItemCount() > 0); I don't think this is correct, ...
7 years, 9 months ago (2013-03-22 19:20:20 UTC) #20
MAD
Thanks! Will re-CQ now... :-) BYE MAD https://codereview.chromium.org/12605010/diff/51001/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/51001/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc#newcode335 chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:335: DCHECK(GetItemCount() > ...
7 years, 9 months ago (2013-03-22 19:26:53 UTC) #21
Alexei Svitkine (slow)
LGTM https://codereview.chromium.org/12605010/diff/51001/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right): https://codereview.chromium.org/12605010/diff/51001/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc#newcode335 chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:335: DCHECK(GetItemCount() > 0); On 2013/03/22 19:26:53, MAD wrote: ...
7 years, 9 months ago (2013-03-22 19:39:39 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/12605010/56001
7 years, 9 months ago (2013-03-22 19:47:10 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/12605010/56001
7 years, 9 months ago (2013-03-23 15:05:44 UTC) #24
commit-bot: I haz the power
7 years, 9 months ago (2013-03-23 21:36:52 UTC) #25
Message was sent while issue was closed.
Change committed as 190084

Powered by Google App Engine