Properly set ShelfAlignment during login

ash/shelf/shelf_layout_manager.cc

Index: ash/shelf/shelf_layout_manager.cc
diff --git a/ash/shelf/shelf_layout_manager.cc b/ash/shelf/shelf_layout_manager.cc
index 5846b634f59b5157c20581b9caee2b3cf7a96291..8f0785070b3b3fd2bd511db6ea38f83be2184b41 100644
--- a/ash/shelf/shelf_layout_manager.cc
+++ b/ash/shelf/shelf_layout_manager.cc
@@ -246,13 +246,11 @@ bool ShelfLayoutManager::SetAlignment(ShelfAlignment 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);
@@ -262,11 +260,8 @@ bool ShelfLayoutManager::SetAlignment(ShelfAlignment alignment) {
 
 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_;
 }
@@ -1159,10 +1154,25 @@ void ShelfLayoutManager::SessionStateChanged(
 }
 
 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