TabbedPane: refactor away manual layout and selected_tab_index_

TabbedPane: refactor away manual layout and selected_tab_index_

This change:
1) Makes TabStrip use a LayoutManager and removes TabStrip::Layout;
2) Makes TabStrip use an EmptyBorder for insets and removes most of
   TabStrip::GetPreferredSize;
3) Replaces TabStrip and MdTabStrip's OnPaint overrides with
   OnPaintBorder overrides to paint their custom borders;
4) Removes the concept of a selected tab index from TabbedPane,
   replacing it with directly querying the TabStrip;
5) Renames TabbedPane::selected_tab_index() to
   TabbedPane::GetSelectedTabIndex() and changes all uses of it.

BUG=635176
Committed: https://crrev.com/2941cbd2032c2cc48b610f7b529a1060bbe4f089
Cr-Commit-Position: refs/heads/master@{#424996}

[Elly Fong-Jones, 2016-10-12 17:11:59]

Description was changed from

==========
TabbedPane: refactor away manual layout and selected_tab_index_

This change:
1) Makes TabStrip use a LayoutManager and removes TabStrip::Layout;
2) Makes TabStrip use an EmptyBorder for insets and removes most of
   TabStrip::GetPreferredSize;
3) Replaces TabStrip and MdTabStrip's OnPaint overrides with
   OnPaintBorder overrides to paint their custom borders;
4) Removes the concept of a selected tab index from TabbedPane,
   replacing it with directly querying the TabStrip;
5) Renames TabbedPane::selected_tab_index() to
   TabbedPane::GetSelectedTabIndex() and changes all uses of it.

BUG=635176
==========

to

==========
TabbedPane: refactor away manual layout and selected_tab_index_

This change:
1) Makes TabStrip use a LayoutManager and removes TabStrip::Layout;
2) Makes TabStrip use an EmptyBorder for insets and removes most of
   TabStrip::GetPreferredSize;
3) Replaces TabStrip and MdTabStrip's OnPaint overrides with
   OnPaintBorder overrides to paint their custom borders;
4) Removes the concept of a selected tab index from TabbedPane,
   replacing it with directly querying the TabStrip;
5) Renames TabbedPane::selected_tab_index() to
   TabbedPane::GetSelectedTabIndex() and changes all uses of it.

BUG=635176
==========

[Elly Fong-Jones, 2016-10-12 17:12:00]

ellyjones@chromium.org changed reviewers:
+ sky@chromium.org

[Elly Fong-Jones, 2016-10-12 17:12:06]

sky: ptal? :)

[sky, 2016-10-12 19:25:56]

sky@chromium.org changed reviewers:
+ msw@chromium.org

[sky, 2016-10-12 19:25:57]

sky->msw as msw did the most recent version.

[msw, 2016-10-12 20:18:35]

lgtm with nits

https://codereview.chromium.org/2415633002/diff/1/ui/views/controls/tabbed_pa...
File ui/views/controls/tabbed_pane/tabbed_pane.cc (right):

https://codereview.chromium.org/2415633002/diff/1/ui/views/controls/tabbed_pa...
ui/views/controls/tabbed_pane/tabbed_pane.cc:61: gfx::Size GetPreferredSize()
const override;
nit: order above OnPaintBorder to match base class decl order.

https://codereview.chromium.org/2415633002/diff/1/ui/views/controls/tabbed_pa...
ui/views/controls/tabbed_pane/tabbed_pane.cc:119:
SetBorder(Border::CreateEmptyBorder(gfx::Insets(5, 10)));
nit: please use constants defined here or bonus points for finding shared
ui/views/layout/layout_constants.h or similar.

https://codereview.chromium.org/2415633002/diff/1/ui/views/controls/tabbed_pa...
ui/views/controls/tabbed_pane/tabbed_pane.cc:303: BoxLayout* layout = new
BoxLayout(BoxLayout::kHorizontal, 9, 0, 0);
ditto nit: 9 -> constant, shared if possible.

https://codereview.chromium.org/2415633002/diff/1/ui/views/controls/tabbed_pa...
ui/views/controls/tabbed_pane/tabbed_pane.cc:383: void
MdTabStrip::OnPaintBorder(gfx::Canvas* canvas) { }
nit: {}

https://codereview.chromium.org/2415633002/diff/1/ui/views/controls/tabbed_pa...
ui/views/controls/tabbed_pane/tabbed_pane.cc:474:
SelectTab(tab_strip_->GetTabAtDeltaFromSelected(delta));
nit: it might make sense to just keep this inlined until its needed elsewhere.

[Elly Fong-Jones, 2016-10-12 21:23:05]

https://codereview.chromium.org/2415633002/diff/1/ui/views/controls/tabbed_pa...
File ui/views/controls/tabbed_pane/tabbed_pane.cc (right):

https://codereview.chromium.org/2415633002/diff/1/ui/views/controls/tabbed_pa...
ui/views/controls/tabbed_pane/tabbed_pane.cc:61: gfx::Size GetPreferredSize()
const override;
On 2016/10/12 20:18:35, msw wrote:
> nit: order above OnPaintBorder to match base class decl order.

Done.

https://codereview.chromium.org/2415633002/diff/1/ui/views/controls/tabbed_pa...
ui/views/controls/tabbed_pane/tabbed_pane.cc:119:
SetBorder(Border::CreateEmptyBorder(gfx::Insets(5, 10)));
On 2016/10/12 20:18:35, msw wrote:
> nit: please use constants defined here or bonus points for finding shared
> ui/views/layout/layout_constants.h or similar.

Done.

https://codereview.chromium.org/2415633002/diff/1/ui/views/controls/tabbed_pa...
ui/views/controls/tabbed_pane/tabbed_pane.cc:303: BoxLayout* layout = new
BoxLayout(BoxLayout::kHorizontal, 9, 0, 0);
On 2016/10/12 20:18:35, msw wrote:
> ditto nit: 9 -> constant, shared if possible.

Done.

https://codereview.chromium.org/2415633002/diff/1/ui/views/controls/tabbed_pa...
ui/views/controls/tabbed_pane/tabbed_pane.cc:383: void
MdTabStrip::OnPaintBorder(gfx::Canvas* canvas) { }
On 2016/10/12 20:18:35, msw wrote:
> nit: {}

Done.

https://codereview.chromium.org/2415633002/diff/1/ui/views/controls/tabbed_pa...
ui/views/controls/tabbed_pane/tabbed_pane.cc:474:
SelectTab(tab_strip_->GetTabAtDeltaFromSelected(delta));
On 2016/10/12 20:18:35, msw wrote:
> nit: it might make sense to just keep this inlined until its needed elsewhere.

I de-inlined it so TabbedPane could stop dealing with tab indexes as far as
possible, and I think leaving it de-inlined serves that goal better.

[Elly Fong-Jones, 2016-10-12 21:23:21]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2016-10-12 21:23:21]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2415633002/#ps20001
(title: "nits for msw")

[commit-bot: I haz the power, 2016-10-12 21:23:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 21:34:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 21:34:37]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Elly Fong-Jones, 2016-10-12 21:35:49]

sky: ptal as views owner? :)

[sky, 2016-10-13 02:09:32]

Rubber stamp LGTM

[Elly Fong-Jones, 2016-10-13 10:15:14]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2016-10-13 10:15:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 10:21:25]

Description was changed from

==========
TabbedPane: refactor away manual layout and selected_tab_index_

This change:
1) Makes TabStrip use a LayoutManager and removes TabStrip::Layout;
2) Makes TabStrip use an EmptyBorder for insets and removes most of
   TabStrip::GetPreferredSize;
3) Replaces TabStrip and MdTabStrip's OnPaint overrides with
   OnPaintBorder overrides to paint their custom borders;
4) Removes the concept of a selected tab index from TabbedPane,
   replacing it with directly querying the TabStrip;
5) Renames TabbedPane::selected_tab_index() to
   TabbedPane::GetSelectedTabIndex() and changes all uses of it.

BUG=635176
==========

to

==========
TabbedPane: refactor away manual layout and selected_tab_index_

This change:
1) Makes TabStrip use a LayoutManager and removes TabStrip::Layout;
2) Makes TabStrip use an EmptyBorder for insets and removes most of
   TabStrip::GetPreferredSize;
3) Replaces TabStrip and MdTabStrip's OnPaint overrides with
   OnPaintBorder overrides to paint their custom borders;
4) Removes the concept of a selected tab index from TabbedPane,
   replacing it with directly querying the TabStrip;
5) Renames TabbedPane::selected_tab_index() to
   TabbedPane::GetSelectedTabIndex() and changes all uses of it.

BUG=635176
==========

[commit-bot: I haz the power, 2016-10-13 10:21:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-13 10:23:43]

Description was changed from

==========
TabbedPane: refactor away manual layout and selected_tab_index_

This change:
1) Makes TabStrip use a LayoutManager and removes TabStrip::Layout;
2) Makes TabStrip use an EmptyBorder for insets and removes most of
   TabStrip::GetPreferredSize;
3) Replaces TabStrip and MdTabStrip's OnPaint overrides with
   OnPaintBorder overrides to paint their custom borders;
4) Removes the concept of a selected tab index from TabbedPane,
   replacing it with directly querying the TabStrip;
5) Renames TabbedPane::selected_tab_index() to
   TabbedPane::GetSelectedTabIndex() and changes all uses of it.

BUG=635176
==========

to

==========
TabbedPane: refactor away manual layout and selected_tab_index_

This change:
1) Makes TabStrip use a LayoutManager and removes TabStrip::Layout;
2) Makes TabStrip use an EmptyBorder for insets and removes most of
   TabStrip::GetPreferredSize;
3) Replaces TabStrip and MdTabStrip's OnPaint overrides with
   OnPaintBorder overrides to paint their custom borders;
4) Removes the concept of a selected tab index from TabbedPane,
   replacing it with directly querying the TabStrip;
5) Renames TabbedPane::selected_tab_index() to
   TabbedPane::GetSelectedTabIndex() and changes all uses of it.

BUG=635176

Committed: https://crrev.com/2941cbd2032c2cc48b610f7b529a1060bbe4f089
Cr-Commit-Position: refs/heads/master@{#424996}
==========

[commit-bot: I haz the power, 2016-10-13 10:23:44]

Patchset 2 (id:??) landed as
https://crrev.com/2941cbd2032c2cc48b610f7b529a1060bbe4f089
Cr-Commit-Position: refs/heads/master@{#424996}