alternate ntp: favicons and menu width enhancements for "Recent tabs" menu

alternate ntp: favicons and menu width enhancements for "Recent tabs" menu

- limit width of recent tabs menu to 320px
- add favicon for devices
- get synced tab favicon first, and only fetch from local history if it's not
  available
  * synced tab favicon is available if sync-tab-favicons command switch is on
  * even if tab's url has not been loaded in local history, if it has synced
    favicon, the favicon will still show

BUG=160703
TEST=verify per bug rpt


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

[sky, 2012-11-13 17:41:49]

https://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right):

https://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:381: void
RecentTabsSubMenuModel::AddTabFavicon(int model_index, int command_id,
nit: each param on its own line.

https://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:390:
SetIcon(index_in_menu, gfx::Image(reinterpret_cast<const unsigned char*>(
You sure you need reinterpret cast here?

https://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right):

https://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:90: gfx::Image
laptop_favicon_;
Is it really worth caching these? ResourceBundle lookup is quite fast.

https://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/views/wr...
File chrome/browser/ui/views/wrench_menu.cc (right):

https://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/views/wr...
chrome/browser/ui/views/wrench_menu.cc:742: views::SubmenuView* submenu =
menu_item_->GetSubmenu();
Instead of checking the command make this take the MenuItem and compare against
that. It's safer.

[kuan, 2012-11-13 18:52:52]

i've addressed all comments in patch set 4.  ptal.  thx.

http://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/toolbar/r...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right):

http://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/toolbar/r...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:381: void
RecentTabsSubMenuModel::AddTabFavicon(int model_index, int command_id,
On 2012/11/13 17:41:49, sky wrote:
> nit: each param on its own line.

Done.

http://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/toolbar/r...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:390:
SetIcon(index_in_menu, gfx::Image(reinterpret_cast<const unsigned char*>(
On 2012/11/13 17:41:49, sky wrote:
> You sure you need reinterpret cast here?

unfortunately yes, it won't build 'cos std::string::data() is const char*
whereas gfx::Image expects a const unsigned char*; it complains "invalid type"

http://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/toolbar/r...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right):

http://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/toolbar/r...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:90: gfx::Image
laptop_favicon_;
On 2012/11/13 17:41:49, sky wrote:
> Is it really worth caching these? ResourceBundle lookup is quite fast.

Done.

http://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/views/wre...
File chrome/browser/ui/views/wrench_menu.cc (right):

http://codereview.chromium.org/11410067/diff/2002/chrome/browser/ui/views/wre...
chrome/browser/ui/views/wrench_menu.cc:742: views::SubmenuView* submenu =
menu_item_->GetSubmenu();
On 2012/11/13 17:41:50, sky wrote:
> Instead of checking the command make this take the MenuItem and compare
against
> that. It's safer.

Done.

[dhollowa, 2012-11-13 20:14:20]

LGTM w/ nits.

http://codereview.chromium.org/11410067/diff/6002/chrome/browser/ui/toolbar/r...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right):

http://codereview.chromium.org/11410067/diff/6002/chrome/browser/ui/toolbar/r...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:383: LOG(ERROR) << "kk
got synced favicon for " << url.spec();
Remove LOG.

http://codereview.chromium.org/11410067/diff/6002/chrome/browser/ui/views/wre...
File chrome/browser/ui/views/wrench_menu.cc (right):

http://codereview.chromium.org/11410067/diff/6002/chrome/browser/ui/views/wre...
chrome/browser/ui/views/wrench_menu.cc:739: // Return the specific menu width of
recent tab menu item if |comamnd_id|
nit: |command_id|.

[kuan, 2012-11-13 20:49:10]

i've addressed all comments in patch set 5; #6 is just grammar fix.  ptal.  thx.

http://codereview.chromium.org/11410067/diff/6002/chrome/browser/ui/toolbar/r...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc (right):

http://codereview.chromium.org/11410067/diff/6002/chrome/browser/ui/toolbar/r...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc:383: LOG(ERROR) << "kk
got synced favicon for " << url.spec();
On 2012/11/13 20:14:20, dhollowa wrote:
> Remove LOG.

Done.

http://codereview.chromium.org/11410067/diff/6002/chrome/browser/ui/views/wre...
File chrome/browser/ui/views/wrench_menu.cc (right):

http://codereview.chromium.org/11410067/diff/6002/chrome/browser/ui/views/wre...
chrome/browser/ui/views/wrench_menu.cc:739: // Return the specific menu width of
recent tab menu item if |comamnd_id|
On 2012/11/13 20:14:20, dhollowa wrote:
> nit: |command_id|.

Done.

[sky, 2012-11-13 22:14:34]

LGTM

[commit-bot: I haz the power, 2012-11-13 22:16:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/11410067/1005

[commit-bot: I haz the power, 2012-11-14 00:55:25]

Change committed as 167543