More cleanup in window_sizer/window_sizer_ash

chrome/browser/ui/window_sizer/window_sizer.cc

Index: chrome/browser/ui/window_sizer/window_sizer.cc
diff --git a/chrome/browser/ui/window_sizer/window_sizer.cc b/chrome/browser/ui/window_sizer/window_sizer.cc
index a2c6635bb89ea2a7cb36bcbbd8e6569934ab5346..80f5fd0ca269bfba27187f5b4817c66ca88e94a4 100644
--- a/chrome/browser/ui/window_sizer/window_sizer.cc
+++ b/chrome/browser/ui/window_sizer/window_sizer.cc
@@ -193,11 +193,13 @@ void WindowSizer::DetermineWindowBoundsAndShowState(
   if (bounds->IsEmpty()) {
 #if defined(USE_ASH)
     // See if ash should decide the window placement.
-    // TODO(beng): insufficient but currently necessary.
-    // http://crbug.com/133312
-    if (chrome::ShouldOpenAshOnStartup() &&
-        GetBoundsOverrideAsh(bounds, show_state))
+    if (IsTabbedBrowserInAsh()) {
+      GetBoundsOverrideAsh(bounds, show_state);
       return;
+    } else if (browser_ && browser_->host_desktop_type() ==
+               chrome::HOST_DESKTOP_TYPE_ASH) {

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

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?

[sky, 2013/09/03 15:58:36]

Not sure either.

[oshima, 2013/09/03 18:15:27]

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

+      GetSavedWindowBounds(bounds, show_state);
+    }
 #endif
     // See if there's last active window's placement information.
     if (GetLastWindowBounds(bounds, show_state))
@@ -213,9 +215,7 @@ void WindowSizer::DetermineWindowBoundsAndShowState(
     // In case of a popup with an 'unspecified' location in ash, we are
     // looking for a good screen location. We are interpreting (0,0) as an
     // unspecified location.
-    if (chrome::ShouldOpenAshOnStartup() &&
-        browser_ && browser_->is_type_popup() &&
-        bounds->x() == 0 && bounds->y() == 0) {
+    if (IsPopupBrowserInAsh() && bounds->origin().IsOrigin()) {
       *bounds = ChromeShellDelegate::instance()->window_positioner()->
           GetPopupPosition(*bounds);
       return;
@@ -399,3 +399,19 @@ ui::WindowShowState WindowSizer::GetWindowDefaultShowState() const {
   // Otherwise we use the default which can be overridden later on.
   return ui::SHOW_STATE_DEFAULT;
 }
+
+bool WindowSizer::IsTabbedBrowserInAsh() const {
+  // TODO(beng): insufficient but currently necessary. http://crbug.com/133312
+  return chrome::ShouldOpenAshOnStartup() &&

[sky, 2013/09/03 15:58:36]

Why is this and the next checking ShouldOpenAshOnStartup?

[oshima, 2013/09/03 18:15:27]

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.

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

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:56]

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

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

Isn't the host_desktop_type enough?

+      browser_ &&
+      browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH &&
+      browser_->is_type_tabbed();
+}
+
+bool WindowSizer::IsPopupBrowserInAsh() const {
+  // TODO(beng): insufficient but currently necessary. http://crbug.com/133312
+  return chrome::ShouldOpenAshOnStartup() &&
+      browser_ &&
+      browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH &&
+      browser_->is_type_popup();
+}