cros-md: Remove the non-MD immersive mode code paths

cros-md: Remove the non-MD immersive mode code paths

Changes:
Remove the non-MD immersive mode code paths

BUG=690295
BUG=685831
TEST=emulator test, automation tests, also add test coverage

Review-Url: https://codereview.chromium.org/2690443002
Cr-Commit-Position: refs/heads/master@{#451022}
Committed: https://chromium.googlesource.com/chromium/src/+/f05f264d80c72ec2681cbd4b3854ab6ceceaee67

[Qiang(Joe) Xu, 2017-02-09 21:22:37]

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

[commit-bot: I haz the power, 2017-02-09 21:23:34]

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, 2017-02-09 22:10:57]

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

[commit-bot: I haz the power, 2017-02-09 22:10:58]

Dry run: This issue passed the CQ dry run.

[Qiang(Joe) Xu, 2017-02-09 23:35:35]

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

[commit-bot: I haz the power, 2017-02-09 23:36:16]

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

[Qiang(Joe) Xu, 2017-02-09 23:38:21]

Description was changed from

==========
cros-md: Fix incorrect top container bounds when tab strip is visible for
immersive fullscreen

BUG=
==========

to

==========
cros-md: Fix incorrect top container bounds when tab strip is visible for
immersive fullscreen

Changes:
When immersive fullscreen, tabstrip is now considered as visible for tab
dragging issues. The top container view height should be zero (before this CL
because the child view (tabstrip) is visible, it is set to the height of
tabstrip), because it should be completely offscreen.

BUG=690295
TEST=emulator test, also add test coverage
==========

[Qiang(Joe) Xu, 2017-02-09 23:38:21]

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

[Qiang(Joe) Xu, 2017-02-09 23:38:39]

Hi sky@, ptal, thanks!

[commit-bot: I haz the power, 2017-02-10 00:20:47]

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

[commit-bot: I haz the power, 2017-02-10 00:20:48]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-10 17:44:51]

One question. I think we should be doing exactly what we were doing pre-md for
the tabstrip. By that I mean making it visible (even when immersive, just not
actually shown on screen). Is this the last change that returns us to the old
pre-md behavior? Are there others?

[Qiang(Joe) Xu, 2017-02-10 17:55:14]

On 2017/02/10 17:44:51, sky wrote:
> One question. I think we should be doing exactly what we were doing pre-md for
> the tabstrip. By that I mean making it visible (even when immersive, just not
> actually shown on screen). Is this the last change that returns us to the old
> pre-md behavior? Are there others?

I would say this may not be the last place. Because making tabstrip visible is a
very fundamental change. Any places that reply on IsTabStripVisible() or
visible() when the view* is tabstrip "may" have problem. Sadly, there are no
tests catching them (if they exist) right now.

Shall I investigate it right now or open a new bug tracking this?

Since making tabstrip visible is merged to M57. This should be a high priority I
believe.

[sky, 2017-02-10 20:42:18]

On Fri, Feb 10, 2017 at 9:55 AM,  <warx@chromium.org> wrote:
> On 2017/02/10 17:44:51, sky wrote:
>> One question. I think we should be doing exactly what we were doing pre-md
>> for
>> the tabstrip. By that I mean making it visible (even when immersive, just
>> not
>> actually shown on screen). Is this the last change that returns us to the
>> old
>> pre-md behavior? Are there others?
>
> I would say this may not be the last place. Because making tabstrip visible
> is a
> very fundamental change.

Can you clarify that? The tabstrip was visible before md, right?
Shouldn't this just be a matter of making sure we share more code
between the two?

  -Scott

> Any places that reply on IsTabStripVisible() or
> visible() when the view* is tabstrip "may" have problem. Sadly, there are no
> tests catching them (if they exist) right now.
>
> Shall I investigate it right now or open a new bug tracking this?
>
> Since making tabstrip visible is merged to M57. This should be a high
> priority I
> believe.
>
> https://codereview.chromium.org/2690443002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Qiang(Joe) Xu, 2017-02-10 23:31:06]

On 2017/02/10 20:42:18, sky wrote:
> On Fri, Feb 10, 2017 at 9:55 AM,  <mailto:warx@chromium.org> wrote:
> > On 2017/02/10 17:44:51, sky wrote:
> >> One question. I think we should be doing exactly what we were doing pre-md
> >> for
> >> the tabstrip. By that I mean making it visible (even when immersive, just
> >> not
> >> actually shown on screen). Is this the last change that returns us to the
> >> old
> >> pre-md behavior? Are there others?
> >
> > I would say this may not be the last place. Because making tabstrip visible
> > is a
> > very fundamental change.
> 
> Can you clarify that? The tabstrip was visible before md, right?
> Shouldn't this just be a matter of making sure we share more code
> between the two?
> 
>   -Scott
> 
> > Any places that reply on IsTabStripVisible() or
> > visible() when the view* is tabstrip "may" have problem. Sadly, there are no
> > tests catching them (if they exist) right now.
> >
> > Shall I investigate it right now or open a new bug tracking this?
> >
> > Since making tabstrip visible is merged to M57. This should be a high
> > priority I
> > believe.
> >
> > https://codereview.chromium.org/2690443002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

hmm, I just saw it. I didn't receive the email notification of this reply.

You are right. Tabstrip is visible because of lightbar style pre-md, which is
why in md tabstrip is invisible because we dropped lightbar style.
But lightbar has a height of 3 but for md visible tabstrip, it uses a height of
29 but actually it should be 0, that is why it is broken here.

Any places that relies on width should be fine, no regression.
Any places that relies on height, since it changes from 3 to 29. That is what I
said it "may" break in other places.

[sky, 2017-02-13 16:57:19]

https://codereview.chromium.org/2690443002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/2690443002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_layout.cc:493: if
(immersive_mode_controller_->IsEnabled() &&
How did pre-md immersive inject a height of 3?

[Qiang(Joe) Xu, 2017-02-13 21:21:08]

https://codereview.chromium.org/2690443002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/2690443002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view_layout.cc:493: if
(immersive_mode_controller_->IsEnabled() &&
On 2017/02/13 16:57:19, sky wrote:
> How did pre-md immersive inject a height of 3?

It is because:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_no...,
the call of GetPreferredSize(),
which is
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c...

Thanks for asking me. Before, when I am doing logging, I blamed the height
change from 3 to 29 on tabstrip visible. But actually the root cause is here,
immersive_style_ is always false now (we should remove it actually), which makes
the preferred size using Tab::GetMinimumInactiveSize().height(). We should now
make it switch between 0 and Tab::GetMinimumInactiveSize().height().

Let me think how to do it gracefully. Should we remove |immersive_style_| in
this CL?

[sky, 2017-02-13 23:14:04]

If immersive_style_ isn't needed, by all means remove it here.

On Mon, Feb 13, 2017 at 1:21 PM,  <warx@chromium.org> wrote:
>
>
https://codereview.chromium.org/2690443002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/browser_view_layout.cc (right):
>
>
https://codereview.chromium.org/2690443002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_view_layout.cc:493: if
> (immersive_mode_controller_->IsEnabled() &&
> On 2017/02/13 16:57:19, sky wrote:
>> How did pre-md immersive inject a height of 3?
>
> It is because:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_no...,
> the call of GetPreferredSize(),
> which is
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c...
>
> Thanks for asking me. Before, when I am doing logging, I blamed the
> height change from 3 to 29 on tabstrip visible. But actually the root
> cause is here, immersive_style_ is always false now (we should remove it
> actually), which makes the preferred size using
> Tab::GetMinimumInactiveSize().height(). We should now make it switch
> between 0 and Tab::GetMinimumInactiveSize().height().
>
> Let me think how to do it gracefully. Should we remove
> |immersive_style_| in this CL?
>
> https://codereview.chromium.org/2690443002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Qiang(Joe) Xu, 2017-02-13 23:29:17]

Description was changed from

==========
cros-md: Fix incorrect top container bounds when tab strip is visible for
immersive fullscreen

Changes:
When immersive fullscreen, tabstrip is now considered as visible for tab
dragging issues. The top container view height should be zero (before this CL
because the child view (tabstrip) is visible, it is set to the height of
tabstrip), because it should be completely offscreen.

BUG=690295
TEST=emulator test, also add test coverage
==========

to

==========
cros-md: Fix immersive style always false induced incorrect tabstrip height

Changes:
The pre-md logic when immersive style is false, it returns a height 29 for
tabstrip bounds. This CL is a workaround solution for crbug.com/690295.
Tabstrip::GetPreferredSize should be well handled when removing pre-md code.

BUG=690295
TEST=emulator test, also add test coverage
==========

[Qiang(Joe) Xu, 2017-02-13 23:29:43]

Description was changed from

==========
cros-md: Fix immersive style always false induced incorrect tabstrip height

Changes:
The pre-md logic when immersive style is false, it returns a height 29 for
tabstrip bounds. This CL is a workaround solution for crbug.com/690295.
Tabstrip::GetPreferredSize should be well handled when removing pre-md code.

BUG=690295
TEST=emulator test, also add test coverage
==========

to

==========
cros-md: Fix immersive style always false induced incorrect tabstrip height

Changes:
In pre-md logic when immersive style is false, it returns a height 29 for
tabstrip bounds. This CL is a workaround solution for crbug.com/690295.
Tabstrip::GetPreferredSize should be well handled when removing pre-md code.

BUG=690295
TEST=emulator test, also add test coverage
==========

[Qiang(Joe) Xu, 2017-02-13 23:30:56]

Patchset #3 (id:40001) has been deleted

[Qiang(Joe) Xu, 2017-02-14 05:14:02]

Description was changed from

==========
cros-md: Fix immersive style always false induced incorrect tabstrip height

Changes:
In pre-md logic when immersive style is false, it returns a height 29 for
tabstrip bounds. This CL is a workaround solution for crbug.com/690295.
Tabstrip::GetPreferredSize should be well handled when removing pre-md code.

BUG=690295
TEST=emulator test, also add test coverage
==========

to

==========
cros-md: Remove the pre-md tabstrip lightbar immersive-style code

Changes:
(1) Remove the pre-md tabstrip lightbar immersive-style code
(2) Fix TabStrip::GetPreferredSize for immersive fullscreen unrevealed state.

BUG=690295
TEST=emulator test, also add test coverage
==========

[Qiang(Joe) Xu, 2017-02-14 05:14:02]

warx@chromium.org changed reviewers:
+ tdanderson@chromium.org

[Qiang(Joe) Xu, 2017-02-14 05:15:55]

Patchset #3 (id:60001) has been deleted

[Qiang(Joe) Xu, 2017-02-14 05:18:33]

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

[commit-bot: I haz the power, 2017-02-14 05:18:54]

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

[Qiang(Joe) Xu, 2017-02-14 05:22:33]

Description was changed from

==========
cros-md: Remove the pre-md tabstrip lightbar immersive-style code

Changes:
(1) Remove the pre-md tabstrip lightbar immersive-style code
(2) Fix TabStrip::GetPreferredSize for immersive fullscreen unrevealed state.

BUG=690295
TEST=emulator test, also add test coverage
==========

to

==========
cros-md: Remove the pre-md tabstrip lightbar immersive-style code

Changes:
(1) Remove the pre-md tabstrip lightbar immersive-style code
(2) Fix TabStrip::GetPreferredSize for immersive fullscreen unrevealed state.

BUG=690295
BUG=614453
TEST=emulator test, also add test coverage
==========

[Qiang(Joe) Xu, 2017-02-14 06:05:37]

Hi all, ptal, thanks!

[commit-bot: I haz the power, 2017-02-14 06:28:25]

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

[commit-bot: I haz the power, 2017-02-14 06:28:26]

Dry run: This issue passed the CQ dry run.

[Qiang(Joe) Xu, 2017-02-14 16:43:26]

Description was changed from

==========
cros-md: Remove the pre-md tabstrip lightbar immersive-style code

Changes:
(1) Remove the pre-md tabstrip lightbar immersive-style code
(2) Fix TabStrip::GetPreferredSize for immersive fullscreen unrevealed state.

BUG=690295
BUG=614453
TEST=emulator test, also add test coverage
==========

to

==========
cros-md: Remove the pre-md tabstrip lightbar immersive-style code

Changes:
(1) Remove the pre-md tabstrip lightbar immersive-style code
(2) Fix TabStrip::GetPreferredSize for immersive fullscreen unrevealed state.

BUG=690295
BUG=614453
BUG=685831
TEST=emulator test, automation tests, also add test coverage
==========

[tdanderson, 2017-02-14 17:14:38]

LGTM (with a couple of comments) for the fact that this removes the non-md
immersive mode code though I did not give a thorough review of overall CL
correctness, I will leave that to sky@.

https://codereview.chromium.org/2690443002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/2690443002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:14: #include
"ash/common/material_design/material_design_controller.h"
I think this can be removed now, too

https://codereview.chromium.org/2690443002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h (right):

https://codereview.chromium.org/2690443002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:2: // Use of
this source code is governed by a BSD-style license that can be
nit: please remove BUG=614453 from the CL description, the other two bug numbers
are sufficient for tracking

https://codereview.chromium.org/2690443002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:9: 
One last place it looks like you've missed for full removal of the non-md
immersive mode code is in ShelfLayoutManager::ComputeTargetOpacity().

[Qiang(Joe) Xu, 2017-02-14 17:19:56]

Description was changed from

==========
cros-md: Remove the pre-md tabstrip lightbar immersive-style code

Changes:
(1) Remove the pre-md tabstrip lightbar immersive-style code
(2) Fix TabStrip::GetPreferredSize for immersive fullscreen unrevealed state.

BUG=690295
BUG=614453
BUG=685831
TEST=emulator test, automation tests, also add test coverage
==========

to

==========
cros-md: Remove the pre-md tabstrip lightbar immersive-style code

Changes:
(1) Remove the pre-md tabstrip lightbar immersive-style code
(2) Fix TabStrip::GetPreferredSize for immersive fullscreen unrevealed state.

BUG=690295
BUG=685831
TEST=emulator test, automation tests, also add test coverage
==========

[Qiang(Joe) Xu, 2017-02-14 17:34:33]

Description was changed from

==========
cros-md: Remove the pre-md tabstrip lightbar immersive-style code

Changes:
(1) Remove the pre-md tabstrip lightbar immersive-style code
(2) Fix TabStrip::GetPreferredSize for immersive fullscreen unrevealed state.

BUG=690295
BUG=685831
TEST=emulator test, automation tests, also add test coverage
==========

to

==========
cros-md: Remove the non-MD immersive mode code paths

Changes:
(1) Remove the non-MD immersive mode code paths
(2) Fix TabStrip::GetPreferredSize for immersive fullscreen unrevealed state.

BUG=690295
BUG=685831
TEST=emulator test, automation tests, also add test coverage
==========

[Qiang(Joe) Xu, 2017-02-14 17:36:38]

Hi all, new patch ready, I think it now removes the complete checklist in
crbug.com/685831.

https://codereview.chromium.org/2690443002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/2690443002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:14: #include
"ash/common/material_design/material_design_controller.h"
On 2017/02/14 17:14:38, tdanderson wrote:
> I think this can be removed now, too

Done.

https://codereview.chromium.org/2690443002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h (right):

https://codereview.chromium.org/2690443002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:2: // Use of
this source code is governed by a BSD-style license that can be
On 2017/02/14 17:14:38, tdanderson wrote:
> nit: please remove BUG=614453 from the CL description, the other two bug
numbers
> are sufficient for tracking

Done.

https://codereview.chromium.org/2690443002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h:9: 
On 2017/02/14 17:14:38, tdanderson wrote:
> One last place it looks like you've missed for full removal of the non-md
> immersive mode code is in ShelfLayoutManager::ComputeTargetOpacity().

Done.

[Qiang(Joe) Xu, 2017-02-14 22:04:32]

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

[commit-bot: I haz the power, 2017-02-14 22:05:24]

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

[sky, 2017-02-14 22:40:57]

https://codereview.chromium.org/2690443002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/2690443002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:213:
caption_button_container_->SetVisible(!IsImmersiveFullscreenUnrevealed());
Is this still needed? If so, why? Prior to your change in immersive mode wasn't
this always true for md?

https://codereview.chromium.org/2690443002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/2690443002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_strip.cc:1505: offscreen_ ? 0 :
Tab::GetMinimumInactiveSize().height());
Why do you need to change this? I'm wondering why you can't always return
GetMinimumInactiveSize()?

[commit-bot: I haz the power, 2017-02-14 23:24:24]

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

[commit-bot: I haz the power, 2017-02-14 23:24:25]

Dry run: This issue passed the CQ dry run.

[Qiang(Joe) Xu, 2017-02-15 00:19:55]

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

[commit-bot: I haz the power, 2017-02-15 00:20:55]

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, 2017-02-15 01:15:35]

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

[commit-bot: I haz the power, 2017-02-15 01:15:37]

Dry run: 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...)

[Qiang(Joe) Xu, 2017-02-15 02:19:05]

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

[commit-bot: I haz the power, 2017-02-15 02:20:31]

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, 2017-02-15 02:45:21]

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

[commit-bot: I haz the power, 2017-02-15 02:45:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Qiang(Joe) Xu, 2017-02-15 05:05:12]

Patchset #7 (id:160001) has been deleted

[Qiang(Joe) Xu, 2017-02-15 05:06:14]

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

[commit-bot: I haz the power, 2017-02-15 05:07:22]

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, 2017-02-15 05:54:03]

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

[commit-bot: I haz the power, 2017-02-15 05:54:04]

Dry run: This issue passed the CQ dry run.

[Qiang(Joe) Xu, 2017-02-15 05:57:02]

Hi Scott, please take another look, thanks!

https://codereview.chromium.org/2690443002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/2690443002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:213:
caption_button_container_->SetVisible(!IsImmersiveFullscreenUnrevealed());
On 2017/02/14 22:40:57, sky wrote:
> Is this still needed? If so, why? Prior to your change in immersive mode
wasn't
> this always true for md?

hmm.. sorry about this, we should not need this.

https://codereview.chromium.org/2690443002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/2690443002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_strip.cc:1505: offscreen_ ? 0 :
Tab::GetMinimumInactiveSize().height());
On 2017/02/14 22:40:57, sky wrote:
> Why do you need to change this? I'm wondering why you can't always return
> GetMinimumInactiveSize()?

If always return GetMinimumInactiveSize, that will be 29 as shown here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/layout_constants.cc?ty...,

which will set tabstrip height to 29 here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi...,
because GetBoundsForTabStripInBrowserView calls TabStrip::GetPreferredSize here.

[sky, 2017-02-15 16:30:09]

https://codereview.chromium.org/2690443002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/2690443002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_strip.cc:1505: offscreen_ ? 0 :
Tab::GetMinimumInactiveSize().height());
On 2017/02/15 05:57:02, Qiang(Joe) Xu wrote:
> On 2017/02/14 22:40:57, sky wrote:
> > Why do you need to change this? I'm wondering why you can't always return
> > GetMinimumInactiveSize()?
> 
> If always return GetMinimumInactiveSize, that will be 29 as shown here:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/layout_constants.cc?ty...,
> 
> which will set tabstrip height to 29 here:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi...,
> because GetBoundsForTabStripInBrowserView calls TabStrip::GetPreferredSize
here.

Sure, but why does this matter? Isn't the tabstrip effectively not shown so that
it's height doesn't really matter?

[Qiang(Joe) Xu, 2017-02-15 19:42:12]

Patchset #8 (id:200001) has been deleted

[Qiang(Joe) Xu, 2017-02-15 22:07:19]

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

[commit-bot: I haz the power, 2017-02-15 22:08:44]

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

[Qiang(Joe) Xu, 2017-02-15 22:30:45]

Description was changed from

==========
cros-md: Remove the non-MD immersive mode code paths

Changes:
(1) Remove the non-MD immersive mode code paths
(2) Fix TabStrip::GetPreferredSize for immersive fullscreen unrevealed state.

BUG=690295
BUG=685831
TEST=emulator test, automation tests, also add test coverage
==========

to

==========
cros-md: Remove the non-MD immersive mode code paths

Changes:
Remove the non-MD immersive mode code paths

BUG=690295
BUG=685831
TEST=emulator test, automation tests, also add test coverage
==========

[commit-bot: I haz the power, 2017-02-15 22:37:22]

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

[commit-bot: I haz the power, 2017-02-15 22:37:23]

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_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Qiang(Joe) Xu, 2017-02-15 23:24:14]

Hi Scott, ptal, thanks!

https://codereview.chromium.org/2690443002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/2690443002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/tab_strip.cc:1505: offscreen_ ? 0 :
Tab::GetMinimumInactiveSize().height());
On 2017/02/15 16:30:08, sky wrote:
> On 2017/02/15 05:57:02, Qiang(Joe) Xu wrote:
> > On 2017/02/14 22:40:57, sky wrote:
> > > Why do you need to change this? I'm wondering why you can't always return
> > > GetMinimumInactiveSize()?
> > 
> > If always return GetMinimumInactiveSize, that will be 29 as shown here:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/layout_constants.cc?ty...,
> > 
> > which will set tabstrip height to 29 here:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi...,
> > because GetBoundsForTabStripInBrowserView calls TabStrip::GetPreferredSize
> here.
> 
> Sure, but why does this matter? Isn't the tabstrip effectively not shown so
that
> it's height doesn't really matter?

I updated this patch. Two main changes in new patch: (1) update
ImmersiveModeController::GetTopContainerVerticalOffset, (2) update
BrowserNonClientFrameView::GetTopInset.

I documented BrowserNonClientFrameView::GetTopInset.

https://codereview.chromium.org/2690443002/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/2690443002/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:156:
(visible_fraction_ - 1));
I feel it can be just cleaned as this.

[sky, 2017-02-16 00:23:11]

Nice, LGTM

[Qiang(Joe) Xu, 2017-02-16 00:26:27]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-02-16 00:26:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2690443002/#ps240001
(title: "rebase & code comments")

[commit-bot: I haz the power, 2017-02-16 00:27:53]

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, 2017-02-16 01:55:37]

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

[commit-bot: I haz the power, 2017-02-16 01:55:39]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Qiang(Joe) Xu, 2017-02-16 02:08:49]

The CQ bit was checked by warx@chromium.org

[commit-bot: I haz the power, 2017-02-16 02:10:09]

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, 2017-02-16 02:31:02]

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

[commit-bot: I haz the power, 2017-02-16 02:31:03]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Qiang(Joe) Xu, 2017-02-16 03:38:14]

The CQ bit was checked by warx@chromium.org

[commit-bot: I haz the power, 2017-02-16 03:39:16]

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, 2017-02-16 05:41:49]

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

[commit-bot: I haz the power, 2017-02-16 05:41:50]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Qiang(Joe) Xu, 2017-02-16 17:46:50]

The CQ bit was checked by warx@chromium.org

[commit-bot: I haz the power, 2017-02-16 17:47:49]

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, 2017-02-16 18:43:24]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1487267210883830,
"parent_rev": "94a01733bc67a29e28b1284ded04480e4c23659f", "commit_rev":
"f05f264d80c72ec2681cbd4b3854ab6ceceaee67"}

[commit-bot: I haz the power, 2017-02-16 18:44:09]

Description was changed from

==========
cros-md: Remove the non-MD immersive mode code paths

Changes:
Remove the non-MD immersive mode code paths

BUG=690295
BUG=685831
TEST=emulator test, automation tests, also add test coverage
==========

to

==========
cros-md: Remove the non-MD immersive mode code paths

Changes:
Remove the non-MD immersive mode code paths

BUG=690295
BUG=685831
TEST=emulator test, automation tests, also add test coverage

Review-Url: https://codereview.chromium.org/2690443002
Cr-Commit-Position: refs/heads/master@{#451022}
Committed:
https://chromium.googlesource.com/chromium/src/+/f05f264d80c72ec2681cbd4b3854...
==========

[commit-bot: I haz the power, 2017-02-16 18:44:11]

Committed patchset #9 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/f05f264d80c72ec2681cbd4b3854...