Properly set ShelfAlignment during login

ash/shelf/shelf_layout_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/shelf/shelf_layout_manager.h"
 
 #include <algorithm>
 #include <cmath>
 #include <cstring>
 #include <string>
@@ skipping 223 matching lines @@
 
 bool ShelfLayoutManager::IsVisible() const {
   // status_area_widget() may be NULL during the shutdown.
   return shelf_->status_area_widget() &&
       shelf_->status_area_widget()->IsVisible() &&
       (state_.visibility_state == SHELF_VISIBLE ||
        (state_.visibility_state == SHELF_AUTO_HIDE &&
         state_.auto_hide_state == SHELF_AUTO_HIDE_SHOWN));
 }
 
 bool ShelfLayoutManager::SetAlignment(ShelfAlignment alignment) {

[tdanderson, 2015/02/11 20:32:35]

Does the documentation for SetAlignment() in the header file require any
updating?

[jonross, 2015/02/11 22:17:44]

Nope, the docs already say that the setting can be delayed for locked
alignments.

   if (alignment_ == alignment)
     return false;
 
   alignment_ = alignment;
-  if (Shell::GetInstance()->session_state_delegate()->IsUserSessionBlocked() ||
-      state_.is_adding_user_screen) {
-    // The shelf will itself move to the bottom while locked. If a request is
-    // sent to move while being locked, we postpone the move until the lock
-    // screen goes away.
+  // The shelf will itself move to the bottom while locked or obscured by user
+  // login. If a request is sent to move while being obscured, we postpone the
+  // move until the user session is resumed.
+  if (ShouldOverrideAlignment())
     return false;
-  }
 
   // This should not be called during the lock screen transitions.
   shelf_->SetAlignment(alignment);
   LayoutShelf();
   return true;
 }
 
 ShelfAlignment ShelfLayoutManager::GetAlignment() const {
   // When the screen is locked or a user gets added, the shelf is forced into
-  // bottom alignment. Note: We cannot use state_.is_screen_locked here since
-  // that flag gets set later than the SessionStateDelegate reports a locked
-  // screen which leads in
-  if (Shell::GetInstance()->session_state_delegate()->IsUserSessionBlocked() ||
-      state_.is_adding_user_screen)
+  // bottom alignment.
+  if (ShouldOverrideAlignment())
     return SHELF_ALIGNMENT_BOTTOM;
   return alignment_;
 }
 
 gfx::Rect ShelfLayoutManager::GetIdealBounds() {
   gfx::Rect bounds(
       ScreenUtil::GetDisplayBoundsInParent(shelf_->GetNativeView()));
   int width = 0, height = 0;
   GetShelfSize(&width, &height);
   return SelectValueForShelfAlignment(
@@ skipping 872 matching lines @@
     UpdateShelfVisibilityAfterLoginUIChange();
     return;
   }
   TargetBounds target_bounds;
   CalculateTargetBounds(state_, &target_bounds);
   UpdateBoundsAndOpacity(target_bounds, true, NULL);
   UpdateVisibilityState();
 }
 
 void ShelfLayoutManager::UpdateShelfVisibilityAfterLoginUIChange() {
-  shelf_->SetAlignment(state_.is_adding_user_screen || state_.is_screen_locked ?
-                           SHELF_ALIGNMENT_BOTTOM : alignment_);
+  shelf_->SetAlignment(GetAlignment());
   UpdateVisibilityState();
   LayoutShelf();
 }
 
+bool ShelfLayoutManager::ShouldOverrideAlignment() const {

[tdanderson, 2015/02/11 20:32:35]

I suggest re-naming this method to something of the form Is...() to make it
clear exactly what its return value means just from the method name itself.

[jonross, 2015/02/11 22:17:44]

Done.

+  // When a user session is blocked, the shelf alignment should be forced to the
+  // bottom. It can be blocked by the screen being locked, or when adding a
+  // user. However the session state becomes active at the start of
+  // transitioning to a user session, while the user session is considered
+  // blocked until the full UI is ready. In order for proper layout we stop
+  // overriding alignment during the transition.

[tdanderson, 2015/02/11 20:32:35]

I love that you're adding documentation to the code, but I don't think there
should be any inside of this method. Instead I'd move this elsewhere:

* Move the documentation for what the method does to the header.
* Move the documentation for why you're calling the method to the individual
call sites.

[jonross, 2015/02/11 22:17:44]

Done.

+  return ((Shell::GetInstance()->session_state_delegate()->GetSessionState() !=

[tdanderson, 2015/02/11 20:32:35]

I find this compound expression really difficult to read. I suggest doing one of
two things:

(1) Break up into two or more early returns.
(2) Keep it as is but use local variables to specify the clauses.

My preference is for (1).

[jonross, 2015/02/11 22:17:44]

Done.

+           SessionStateDelegate::SESSION_STATE_ACTIVE) &&
+          (Shell::GetInstance()
+               ->session_state_delegate()
+               ->IsUserSessionBlocked() ||
+           state_.is_adding_user_screen)) ||
+         state_.is_screen_locked;
+}
+
 }  // namespace ash