App List: Refactor ContentsSwitcherView so it doesn't hard-code pages.

App List: Refactor ContentsSwitcherView so it doesn't hard-code pages.

Removed hard-coded addition of start page and apps buttons in the
ContentsSwitcherView constructor. Instead, adding a page to ContentsView
automatically adds the appropriate button to ContentsSwitcherView.

ContentsView and ContentsSwitcherView now include pointers to one
another (so initialization of ContentsView is split into a separate Init
phase). ContentsView::AddLauncherPage now takes a resource ID.

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

[calamity, 2014-06-18 05:22:29]

https://codereview.chromium.org/336313010/diff/10006/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/336313010/diff/10006/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:115: new ContentsView(this,
contents_switcher_view_, model_, delegate_);
I think ContentsSwitcherView should take ContentsView as a constructor param and
not vice versa. It makes sense for a ContentsView to exist without a
ContentsSwitcherView but not the opposite.

You could move the page creation into a ContentsView::InitNamedPages() and have
set_contents_switcher_view().

https://codereview.chromium.org/336313010/diff/10006/ui/app_list/views/conten...
File ui/app_list/views/contents_switcher_view.cc (right):

https://codereview.chromium.org/336313010/diff/10006/ui/app_list/views/conten...
ui/app_list/views/contents_switcher_view.cc:41: if (resource_id)
nit: Braces around multiline statement.

https://codereview.chromium.org/336313010/diff/10006/ui/app_list/views/conten...
File ui/app_list/views/contents_switcher_view.h (right):

https://codereview.chromium.org/336313010/diff/10006/ui/app_list/views/conten...
ui/app_list/views/contents_switcher_view.h:31: void AddSwitcherButton(int
resource_id, int page_index);
Add a note about resource_id of 0 being ignored.

[Matt Giuca, 2014-06-18 06:47:02]

https://codereview.chromium.org/336313010/diff/10006/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/336313010/diff/10006/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:115: new ContentsView(this,
contents_switcher_view_, model_, delegate_);
On 2014/06/18 05:22:29, calamity wrote:
> I think ContentsSwitcherView should take ContentsView as a constructor param
and
> not vice versa. It makes sense for a ContentsView to exist without a
> ContentsSwitcherView but not the opposite.
> 
> You could move the page creation into a ContentsView::InitNamedPages() and
have
> set_contents_switcher_view().

Done. Good suggestion.

https://codereview.chromium.org/336313010/diff/10006/ui/app_list/views/conten...
File ui/app_list/views/contents_switcher_view.cc (right):

https://codereview.chromium.org/336313010/diff/10006/ui/app_list/views/conten...
ui/app_list/views/contents_switcher_view.cc:41: if (resource_id)
On 2014/06/18 05:22:29, calamity wrote:
> nit: Braces around multiline statement.

Done.

https://codereview.chromium.org/336313010/diff/10006/ui/app_list/views/conten...
File ui/app_list/views/contents_switcher_view.h (right):

https://codereview.chromium.org/336313010/diff/10006/ui/app_list/views/conten...
ui/app_list/views/contents_switcher_view.h:31: void AddSwitcherButton(int
resource_id, int page_index);
On 2014/06/18 05:22:29, calamity wrote:
> Add a note about resource_id of 0 being ignored.

Done.

[calamity, 2014-06-19 03:04:30]

lgtm

[Matt Giuca, 2014-06-19 23:13:24]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-06-19 23:15:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mgiuca@chromium.org/336313010/120001

[commit-bot: I haz the power, 2014-06-20 04:44:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-20 11:29:32]

Change committed as 278674