More cleanup in window_sizer/window_sizer_ash

More cleanup in window_sizer/window_sizer_ash
* The bounds obtained from GetBoundsOverrideAsh is ignored
  (overridden) by the following logic if it returns false,
  so skip early for the condition that returns false.
* Removed the code for non tabbed case in GetBoundsOverrideAsh as it won't be used.

* removed fullscreen controller check because it seems to be
  redundant with window->IsFullscreen() check. (IsFullscerenForBrowser() is always false if window->IsFullscreen() is false)

BUG=272460
R=skuhne@chromium.org, sky@chromium.org

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

[oshima, 2013-08-30 23:49:46]

https://codereview.chromium.org/23567007/diff/8001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer_ash.cc (left):

https://codereview.chromium.org/23567007/diff/8001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:109: bool is_popup = browser_
&& browser_->is_type_popup();
apps v1 is TYPE_POPUP, so these checks aren't necessary.

[oshima, 2013-08-31 00:54:56]

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer.cc:200:
chrome::HOST_DESKTOP_TYPE_ASH) {
This may look odd, but WindowSizerAsh::TestShowState fails without
this because condition assumes that show_state is always updated to saved state
before following the standard path.
 I'm not sure if this is expected or a bug. Any idea?

[Mr4D (OOO till 08-26), 2013-09-03 15:55:02]

lgtm

Only a few nits.

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right):

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:93: ui::WindowShowState*
show_state) const {
Since you have changed the meaning of the function - you should rename it to
reflect that. What about:

GetTabbedBoundsOverrideAsh
?

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:136: (!browser_->window() ||
!browser_->window()->IsFullscreen()))
I know you asked me about the fullscreen controller - so you are sure you can
ignore the controller?

[sky, 2013-09-03 15:58:35]

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer.cc:200:
chrome::HOST_DESKTOP_TYPE_ASH) {
On 2013/08/31 00:54:56, oshima wrote:
> This may look odd, but WindowSizerAsh::TestShowState fails without
> this because condition assumes that show_state is always updated to saved
state
> before following the standard path.
>  I'm not sure if this is expected or a bug. Any idea?

Not sure either.

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer.cc:405: return
chrome::ShouldOpenAshOnStartup() &&
Why is this and the next checking ShouldOpenAshOnStartup?

[oshima, 2013-09-03 18:15:26]

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer.cc:200:
chrome::HOST_DESKTOP_TYPE_ASH) {
On 2013/09/03 15:58:36, sky wrote:
> On 2013/08/31 00:54:56, oshima wrote:
> > This may look odd, but WindowSizerAsh::TestShowState fails without
> > this because condition assumes that show_state is always updated to saved
> state
> > before following the standard path.
> >  I'm not sure if this is expected or a bug. Any idea?
> 
> Not sure either.

I'll keep this to make sure the logic stays the same for now.
I also added comment in case someone find this odd.

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer.cc:405: return
chrome::ShouldOpenAshOnStartup() &&
On 2013/09/03 15:58:36, sky wrote:
> Why is this and the next checking ShouldOpenAshOnStartup?

My understanding is that this is necessary for win aura not to use ash logic.
The correct fix would be passing a context like other places,
I guess.

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right):

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:93: ui::WindowShowState*
show_state) const {
On 2013/09/03 15:55:03, Mr4D wrote:
> Since you have changed the meaning of the function - you should rename it to
> reflect that. What about:
> 
> GetTabbedBoundsOverrideAsh
> ?

Changed to GetTabbedBrowserBoundsAsh

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:136: (!browser_->window() ||
!browser_->window()->IsFullscreen()))
On 2013/09/03 15:55:03, Mr4D wrote:
> I know you asked me about the fullscreen controller - so you are sure you can
> ignore the controller?

Yes, i'm pretty sure this isn't necessary. (you can't open a new tabbed browser
with html5's fullscreen status).

[Mr4D (OOO till 08-26), 2013-09-03 18:21:28]

One comment.

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer.cc:405: return
chrome::ShouldOpenAshOnStartup() &&
I might be mistaken - but you should not get here in the first case if it is not
set. E.g. see line 268 above.

[oshima, 2013-09-03 18:36:55]

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer.cc:405: return
chrome::ShouldOpenAshOnStartup() &&
On 2013/09/03 18:21:28, Mr4D wrote:
> I might be mistaken - but you should not get here in the first case if it is
not
> set. E.g. see line 268 above.

Sorry, I didn't get it. What do you mean?

[sky, 2013-09-03 20:47:31]

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer.cc:405: return
chrome::ShouldOpenAshOnStartup() &&
On 2013/09/03 18:15:27, oshima wrote:
> On 2013/09/03 15:58:36, sky wrote:
> > Why is this and the next checking ShouldOpenAshOnStartup?
> 
> My understanding is that this is necessary for win aura not to use ash logic.
> The correct fix would be passing a context like other places,
> I guess.
> 

Isn't the host_desktop_type enough?

[oshima, 2013-09-03 20:59:15]

On 2013/09/03 20:47:31, sky wrote:
>
https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
> File chrome/browser/ui/window_sizer/window_sizer.cc (right):
> 
>
https://codereview.chromium.org/23567007/diff/14001/chrome/browser/ui/window_...
> chrome/browser/ui/window_sizer/window_sizer.cc:405: return
> chrome::ShouldOpenAshOnStartup() &&
> On 2013/09/03 18:15:27, oshima wrote:
> > On 2013/09/03 15:58:36, sky wrote:
> > > Why is this and the next checking ShouldOpenAshOnStartup?
> > 
> > My understanding is that this is necessary for win aura not to use ash
logic.
> > The correct fix would be passing a context like other places,
> > I guess.
> > 
> 
> Isn't the host_desktop_type enough?

This is still necessary because browser_ can be NULL.

[sky, 2013-09-03 23:16:43]

Good point. LGTM`

[commit-bot: I haz the power, 2013-09-04 20:35:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/23567007/32001

[commit-bot: I haz the power, 2013-09-04 20:55:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/23567007/48001

[commit-bot: I haz the power, 2013-09-04 21:19:57]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-09-04 22:59:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/23567007/47001

[commit-bot: I haz the power, 2013-09-04 23:53:29]

Retried try job too often on ios_dbg_simulator for step(s) ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-09-05 01:07:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/23567007/47001

[commit-bot: I haz the power, 2013-09-05 02:20:06]

Retried try job too often on ios_dbg_simulator for step(s) ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[oshima, 2013-09-05 18:19:45]

Committed patchset #9 manually as r221461 (presubmit successful).