Alternate NTP: Limit width of tab titles in recent tabs menu

Alternate NTP: Limit width of tab titles in recent tabs menu

This is the Mac version of this CL:
https://chromiumcodereview.appspot.com/11410067
It limits the width of tab titles and window names in the recent tabs menu.

BUG=160844
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170914

[sail, 2012-11-21 05:46:47]

Hi kuan, I'd like to refactor the views code to use the new helper function too
but I'm not sure how to.

[sail, 2012-11-21 06:31:03]

On 2012/11/21 05:46:47, sail wrote:
> Hi kuan, I'd like to refactor the views code to use the new helper function
too
> but I'm not sure how to.

Oops, I think the above comment was too brief. What I mean is that it looks like
the views code elides all menu items in the Recent Tabs menu, not just the
window and tab names. If this is the case then it might incorrectly elide the
"restore tabs" and "no devices" command.

I think the new RecentTabsSubMenuModel::GetMaxWidthForItemAtIndex() but I'm not
sure how that function can be called from the views code.

[sail, 2012-11-21 06:32:12]

> I think the new RecentTabsSubMenuModel::GetMaxWidthForItemAtIndex() but I'm
not
> sure how that function can be called from the views code.

That should be "I think the new
RecentTabsSubMenuModel::GetMaxWidthForItemAtIndex() function might fix this but
I'm not sure how that function can be called from the views code.

[kuan, 2012-11-21 15:52:43]

u r right, good catch!  my implementation for max menu width also elide
restore-tab and no-devices :(  i'll handle the views part after ur cl lands.

lgtm for chrome/browser/ui/toolbar, just nits.

https://codereview.chromium.org/11316127/diff/1/chrome/browser/ui/cocoa/menu_...
File chrome/browser/ui/cocoa/menu_controller.h (right):

https://codereview.chromium.org/11316127/diff/1/chrome/browser/ui/cocoa/menu_...
chrome/browser/ui/cocoa/menu_controller.h:78: // Returns the maximum width for
the menu menu item. Returns -1 to indicate
nit: remove one of the "menu" words.

https://codereview.chromium.org/11316127/diff/1/chrome/browser/ui/toolbar/rec...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right):

https://codereview.chromium.org/11316127/diff/1/chrome/browser/ui/toolbar/rec...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:242: // Create
1 sessions with 1 window and 1 tab.
nit: s/sessions/session/

[sail, 2012-11-21 19:28:30]

https://codereview.chromium.org/11316127/diff/1/chrome/browser/ui/cocoa/menu_...
File chrome/browser/ui/cocoa/menu_controller.h (right):

https://codereview.chromium.org/11316127/diff/1/chrome/browser/ui/cocoa/menu_...
chrome/browser/ui/cocoa/menu_controller.h:78: // Returns the maximum width for
the menu menu item. Returns -1 to indicate
On 2012/11/21 15:52:43, kuan wrote:
> nit: remove one of the "menu" words.

Done.

https://codereview.chromium.org/11316127/diff/1/chrome/browser/ui/toolbar/rec...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc (right):

https://codereview.chromium.org/11316127/diff/1/chrome/browser/ui/toolbar/rec...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc:242: // Create
1 sessions with 1 window and 1 tab.
On 2012/11/21 15:52:43, kuan wrote:
> nit: s/sessions/session/

Done.

[Robert Sesek, 2012-11-27 17:09:20]

LGTM

https://codereview.chromium.org/11316127/diff/1/chrome/browser/ui/cocoa/wrenc...
File chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.mm (right):

https://codereview.chromium.org/11316127/diff/1/chrome/browser/ui/cocoa/wrenc...
chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.mm:340: if
(recentTabsMenuModel == model)
nit: braces needed since the body is more than one line

[sail, 2012-11-28 07:38:25]

https://codereview.chromium.org/11316127/diff/1/chrome/browser/ui/cocoa/wrenc...
File chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.mm (right):

https://codereview.chromium.org/11316127/diff/1/chrome/browser/ui/cocoa/wrenc...
chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.mm:340: if
(recentTabsMenuModel == model)
On 2012/11/27 17:09:20, rsesek wrote:
> nit: braces needed since the body is more than one line

Done.

[commit-bot: I haz the power, 2012-11-28 07:39:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11316127/12001

[commit-bot: I haz the power, 2012-11-28 07:39:45]

Presubmit check for 11316127-12001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui
    chrome/app

Presubmit checks took 4.3s to calculate.

[sail, 2012-11-28 07:48:03]

+sky for chrome/browser/ui/* OWNERS review

[sky, 2012-11-28 15:41:28]

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
File chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc (right):

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc:60:
SessionID::id_type id;
Add constructor to initialize id.

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc:65: WindowInfo() {}
initialize id.

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc:71: SessionInfo()
{}
initialize id.

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right):

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:240: int
RecentTabsSubMenuModel::GetMaxWidthForItemAtIndex(int item_index) const {
Is there a reason for the method instead of a constant?

[sail, 2012-11-30 23:24:53]

Comments addressed. Please take another look.

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
File chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc (right):

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc:60:
SessionID::id_type id;
On 2012/11/28 15:41:28, sky wrote:
> Add constructor to initialize id.

Done.

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc:65: WindowInfo() {}
On 2012/11/28 15:41:28, sky wrote:
> initialize id.

Done.

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc:71: SessionInfo()
{}
On 2012/11/28 15:41:28, sky wrote:
> initialize id.

Done.

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right):

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:240: int
RecentTabsSubMenuModel::GetMaxWidthForItemAtIndex(int item_index) const {
On 2012/11/28 15:41:28, sky wrote:
> Is there a reason for the method instead of a constant?

Some of the items in this menu should be elided but some shouldn't.

This logic is contained inside this function (the if statement below).

By having this logic in this class we can share it with all platforms. Currently
only the Mac code calls this but eventually the Views code will call this as
well.

[kuan, 2012-11-30 23:40:59]

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right):

https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:240: int
RecentTabsSubMenuModel::GetMaxWidthForItemAtIndex(int item_index) const {
On 2012/11/30 23:24:53, sail wrote:
> On 2012/11/28 15:41:28, sky wrote:
> > Is there a reason for the method instead of a constant?
> 
> Some of the items in this menu should be elided but some shouldn't.
> 
> This logic is contained inside this function (the if statement below).
> 
> By having this logic in this class we can share it with all platforms.
Currently
> only the Mac code calls this but eventually the Views code will call this as
> well.

is it possible to make this a static method that takes in a command_id ?  that
way, the caller doesn't need to static_cast the |MenuModel| to
|RecentTabsSubMenuModel|  to use this method.

[sail, 2012-11-30 23:57:15]

On 2012/11/30 23:40:59, kuan wrote:
>
https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
> File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
> chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:240: int
> RecentTabsSubMenuModel::GetMaxWidthForItemAtIndex(int item_index) const {
> On 2012/11/30 23:24:53, sail wrote:
> > On 2012/11/28 15:41:28, sky wrote:
> > > Is there a reason for the method instead of a constant?
> > 
> > Some of the items in this menu should be elided but some shouldn't.
> > 
> > This logic is contained inside this function (the if statement below).
> > 
> > By having this logic in this class we can share it with all platforms.
> Currently
> > only the Mac code calls this but eventually the Views code will call this as
> > well.
> 
> is it possible to make this a static method that takes in a command_id ?  that
> way, the caller doesn't need to static_cast the |MenuModel| to
> |RecentTabsSubMenuModel|  to use this method.

I think the API is easier to understand as a menu index. It seems like most of
the other menu model functions take a menu index as well.

Instead of doing a static_cast here it might be better to change
WrenchMenu::RecentTabsMenuModelDelegate to explicitly take a
RecentTabsSubMenuModel instance.

I think this make more sense since the delegate is already explicitly a
RecentTabsSubMenuModel delegate. Also, it will make it easier to share
functionality between platforms by moving functionality into
RecentTabsSubMenuModel.

[kuan, 2012-12-01 00:06:07]

On 2012/11/30 23:57:15, sail wrote:
> On 2012/11/30 23:40:59, kuan wrote:
> >
>
https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
> > File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
> > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:240: int
> > RecentTabsSubMenuModel::GetMaxWidthForItemAtIndex(int item_index) const {
> > On 2012/11/30 23:24:53, sail wrote:
> > > On 2012/11/28 15:41:28, sky wrote:
> > > > Is there a reason for the method instead of a constant?
> > > 
> > > Some of the items in this menu should be elided but some shouldn't.
> > > 
> > > This logic is contained inside this function (the if statement below).
> > > 
> > > By having this logic in this class we can share it with all platforms.
> > Currently
> > > only the Mac code calls this but eventually the Views code will call this
as
> > > well.
> > 
> > is it possible to make this a static method that takes in a command_id ? 
that
> > way, the caller doesn't need to static_cast the |MenuModel| to
> > |RecentTabsSubMenuModel|  to use this method.
> 
> I think the API is easier to understand as a menu index. It seems like most of
> the other menu model functions take a menu index as well.
> 
> Instead of doing a static_cast here it might be better to change
> WrenchMenu::RecentTabsMenuModelDelegate to explicitly take a
> RecentTabsSubMenuModel instance.
> 
> I think this make more sense since the delegate is already explicitly a
> RecentTabsSubMenuModel delegate. Also, it will make it easier to share
> functionality between platforms by moving functionality into
> RecentTabsSubMenuModel.

yes, i do intend to pass in a RecentTabsSubMenuModel into
WrenchMenu::RecentTabsMenuModelDelegate; however it still needs a static_cast
from MenuModel to RecentTabsSubMenuModel when i instantiate the delegate in
WrenchMenu.  i'm ok with it if static_cast is ok.

[sail, 2012-12-01 00:11:01]

On 2012/12/01 00:06:07, kuan wrote:
> On 2012/11/30 23:57:15, sail wrote:
> > On 2012/11/30 23:40:59, kuan wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
> > > File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
> > > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:240: int
> > > RecentTabsSubMenuModel::GetMaxWidthForItemAtIndex(int item_index) const {
> > > On 2012/11/30 23:24:53, sail wrote:
> > > > On 2012/11/28 15:41:28, sky wrote:
> > > > > Is there a reason for the method instead of a constant?
> > > > 
> > > > Some of the items in this menu should be elided but some shouldn't.
> > > > 
> > > > This logic is contained inside this function (the if statement below).
> > > > 
> > > > By having this logic in this class we can share it with all platforms.
> > > Currently
> > > > only the Mac code calls this but eventually the Views code will call
this
> as
> > > > well.
> > > 
> > > is it possible to make this a static method that takes in a command_id ? 
> that
> > > way, the caller doesn't need to static_cast the |MenuModel| to
> > > |RecentTabsSubMenuModel|  to use this method.
> > 
> > I think the API is easier to understand as a menu index. It seems like most
of
> > the other menu model functions take a menu index as well.
> > 
> > Instead of doing a static_cast here it might be better to change
> > WrenchMenu::RecentTabsMenuModelDelegate to explicitly take a
> > RecentTabsSubMenuModel instance.
> > 
> > I think this make more sense since the delegate is already explicitly a
> > RecentTabsSubMenuModel delegate. Also, it will make it easier to share
> > functionality between platforms by moving functionality into
> > RecentTabsSubMenuModel.
> 
> yes, i do intend to pass in a RecentTabsSubMenuModel into
> WrenchMenu::RecentTabsMenuModelDelegate; however it still needs a static_cast
> from MenuModel to RecentTabsSubMenuModel when i instantiate the delegate in
> WrenchMenu.  i'm ok with it if static_cast is ok.

Yea, I think static cast would be preferable. Specially if we move the static
cast to the RecentTabsMenuModelDelegate constructor.

[sail, 2012-12-01 00:11:01]

On 2012/12/01 00:06:07, kuan wrote:
> On 2012/11/30 23:57:15, sail wrote:
> > On 2012/11/30 23:40:59, kuan wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
> > > File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/11316127/diff/12001/chrome/browser/ui/...
> > > chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:240: int
> > > RecentTabsSubMenuModel::GetMaxWidthForItemAtIndex(int item_index) const {
> > > On 2012/11/30 23:24:53, sail wrote:
> > > > On 2012/11/28 15:41:28, sky wrote:
> > > > > Is there a reason for the method instead of a constant?
> > > > 
> > > > Some of the items in this menu should be elided but some shouldn't.
> > > > 
> > > > This logic is contained inside this function (the if statement below).
> > > > 
> > > > By having this logic in this class we can share it with all platforms.
> > > Currently
> > > > only the Mac code calls this but eventually the Views code will call
this
> as
> > > > well.
> > > 
> > > is it possible to make this a static method that takes in a command_id ? 
> that
> > > way, the caller doesn't need to static_cast the |MenuModel| to
> > > |RecentTabsSubMenuModel|  to use this method.
> > 
> > I think the API is easier to understand as a menu index. It seems like most
of
> > the other menu model functions take a menu index as well.
> > 
> > Instead of doing a static_cast here it might be better to change
> > WrenchMenu::RecentTabsMenuModelDelegate to explicitly take a
> > RecentTabsSubMenuModel instance.
> > 
> > I think this make more sense since the delegate is already explicitly a
> > RecentTabsSubMenuModel delegate. Also, it will make it easier to share
> > functionality between platforms by moving functionality into
> > RecentTabsSubMenuModel.
> 
> yes, i do intend to pass in a RecentTabsSubMenuModel into
> WrenchMenu::RecentTabsMenuModelDelegate; however it still needs a static_cast
> from MenuModel to RecentTabsSubMenuModel when i instantiate the delegate in
> WrenchMenu.  i'm ok with it if static_cast is ok.

Yea, I think static cast would be preferable. Specially if we move the static
cast to the RecentTabsMenuModelDelegate constructor.

[sail, 2012-12-03 22:43:49]

sky: Ping!

[sky, 2012-12-03 23:56:25]

LGTM

[commit-bot: I haz the power, 2012-12-04 00:28:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11316127/7002

[commit-bot: I haz the power, 2012-12-04 04:21:15]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-04 04:22:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11316127/7002

[commit-bot: I haz the power, 2012-12-04 07:20:17]

Retried try job too often on mac_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-04 07:40:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11316127/7002

[commit-bot: I haz the power, 2012-12-04 09:40:46]

Change committed as 170914