Remove pre-MD code from browser/ui/views/tabs/tab.cc

Remove pre-MD code from browser/ui/views/tabs/tab.cc

BUG=648281
TBR=oshima@chromium.org

Committed: https://crrev.com/bac57d0ba546705cb0987bd53138ac1172770bbb
Cr-Commit-Position: refs/heads/master@{#421027}

[Evan Stade, 2016-09-23 21:47:09]

Description was changed from

==========
Remove pre-MD code from browser/ui/views/tabs/tab.cc

BUG=648281
==========

to

==========
Remove pre-MD code from browser/ui/views/tabs/tab.cc

I attempted to only change tab.cc/h, but had to delete
some immersive mode code too (that mode is now deprecated).
I tried to delete the minimum amount of immersive related
code.

BUG=648281
==========

[Evan Stade, 2016-09-23 21:47:09]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org, tdanderson@chromium.org

[Evan Stade, 2016-09-23 21:50:19]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 21:50:38]

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

[Evan Stade, 2016-09-23 21:55:33]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 21:56:00]

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

[tdanderson, 2016-09-23 21:57:10]

https://codereview.chromium.org/2368653002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (left):

https://codereview.chromium.org/2368653002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:205: if
(ash::MaterialDesignController::IsImmersiveModeMaterial())
MD immersive mode was turned on in early 55 which hasn't even hit beta channel,
so please leave the old immersive mode code in place until MD immersive makes it
into stable. We will clean it up at that point as per the TODO above.

[commit-bot: I haz the power, 2016-09-23 22:45:49]

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

[commit-bot: I haz the power, 2016-09-23 22:45:50]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-09-24 06:02:44]

I'm going to wait for another ping before reviewing this, as it sounds as if
restoring some of this immersive mode stuff may be nontrivial.

[Evan Stade, 2016-09-26 16:33:18]

Description was changed from

==========
Remove pre-MD code from browser/ui/views/tabs/tab.cc

I attempted to only change tab.cc/h, but had to delete
some immersive mode code too (that mode is now deprecated).
I tried to delete the minimum amount of immersive related
code.

BUG=648281
==========

to

==========
Remove pre-MD code from browser/ui/views/tabs/tab.cc

BUG=648281
==========

[Evan Stade, 2016-09-26 16:40:05]

Description was changed from

==========
Remove pre-MD code from browser/ui/views/tabs/tab.cc

BUG=648281
==========

to

==========
Remove pre-MD code from browser/ui/views/tabs/tab.cc

BUG=648281
TBR=oshima@chromium.org
==========

[Evan Stade, 2016-09-26 16:40:05]

estade@chromium.org changed reviewers:
+ oshima@chromium.org
- tdanderson@chromium.org

[Evan Stade, 2016-09-26 16:40:39]

Leaving immersive mode code in place. tdanderson to cc, +oshima tbr for asset
deletion

https://codereview.chromium.org/2368653002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (left):

https://codereview.chromium.org/2368653002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:205: if
(ash::MaterialDesignController::IsImmersiveModeMaterial())
On 2016/09/23 21:57:09, tdanderson (OOO until 9-29) wrote:
> MD immersive mode was turned on in early 55 which hasn't even hit beta
channel,
> so please leave the old immersive mode code in place until MD immersive makes
it
> into stable. We will clean it up at that point as per the TODO above.

ah ok, my bad.

[Evan Stade, 2016-09-26 16:40:45]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 16:41:19]

Dry run: 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-09-26 18:00:14]

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

[commit-bot: I haz the power, 2016-09-26 18:00:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Peter Kasting, 2016-09-26 21:05:29]

https://codereview.chromium.org/2368653002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2368653002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab.cc:1582: gfx::Rect contents =
GetContentsBounds();
This seems to be a behavior change?  The old code, as the comment describes,
used the width of the top of the tab.  The new code uses the width of the whole
tab rect (i.e. the width of the bottom of the tab), so the comment is no longer
correct.  I'm worried that the behavior change isn't right either?

[Evan Stade, 2016-09-26 21:57:34]

https://codereview.chromium.org/2368653002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2368653002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab.cc:1582: gfx::Rect contents =
GetContentsBounds();
On 2016/09/26 21:05:29, Peter Kasting wrote:
> This seems to be a behavior change?  The old code, as the comment describes,
> used the width of the top of the tab.  The new code uses the width of the
whole
> tab rect (i.e. the width of the bottom of the tab), so the comment is no
longer
> correct.  I'm worried that the behavior change isn't right either?

I don't think so --- the tab's border is set here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab.cc?rcl=...

GetLayoutInsets(TAB) to my understanding describes the insets for the portion of
the tab where we put the icon, text, etc., i.e. the width matches the tab's top
stroke.

[Peter Kasting, 2016-09-26 22:02:42]

LGTM

https://codereview.chromium.org/2368653002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2368653002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab.cc:1582: gfx::Rect contents =
GetContentsBounds();
On 2016/09/26 21:57:34, Evan Stade wrote:
> On 2016/09/26 21:05:29, Peter Kasting wrote:
> > This seems to be a behavior change?  The old code, as the comment describes,
> > used the width of the top of the tab.  The new code uses the width of the
> whole
> > tab rect (i.e. the width of the bottom of the tab), so the comment is no
> longer
> > correct.  I'm worried that the behavior change isn't right either?
> 
> I don't think so --- the tab's border is set here:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab.cc?rcl=...
> 
> GetLayoutInsets(TAB) to my understanding describes the insets for the portion
of
> the tab where we put the icon, text, etc., i.e. the width matches the tab's
top
> stroke.

Oh, I was thinking GetLocalBounds(), not GetContentsBounds().

Hmm.  We might be off by a little bit (roughly, the kBarPadding amount in the
old code).  But this is at least approximately correct.

I seem to recall being told by Scott not to worry about getting immersive mode
behavior perfect in MD anyway, so this seems OK.

[Evan Stade, 2016-09-26 22:11:13]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-09-26 22:12:02]

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-09-26 22:48:01]

Description was changed from

==========
Remove pre-MD code from browser/ui/views/tabs/tab.cc

BUG=648281
TBR=oshima@chromium.org
==========

to

==========
Remove pre-MD code from browser/ui/views/tabs/tab.cc

BUG=648281
TBR=oshima@chromium.org
==========

[commit-bot: I haz the power, 2016-09-26 22:48:02]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-26 22:50:09]

Description was changed from

==========
Remove pre-MD code from browser/ui/views/tabs/tab.cc

BUG=648281
TBR=oshima@chromium.org
==========

to

==========
Remove pre-MD code from browser/ui/views/tabs/tab.cc

BUG=648281
TBR=oshima@chromium.org

Committed: https://crrev.com/bac57d0ba546705cb0987bd53138ac1172770bbb
Cr-Commit-Position: refs/heads/master@{#421027}
==========

[commit-bot: I haz the power, 2016-09-26 22:50:10]

Patchset 6 (id:??) landed as
https://crrev.com/bac57d0ba546705cb0987bd53138ac1172770bbb
Cr-Commit-Position: refs/heads/master@{#421027}