[Android History] List item shadows and width constrained styling

chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java

Index: chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java b/chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java
index dd4d18a02fad29538fe77e97e3833267087db8a2..1708d8cd17985ecf44b5e05813a5776e3208530c 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java
@@ -31,6 +31,8 @@ import org.chromium.chrome.R;
 import org.chromium.chrome.browser.widget.NumberRollView;
 import org.chromium.chrome.browser.widget.TintedDrawable;
 import org.chromium.chrome.browser.widget.TintedImageButton;
+import org.chromium.chrome.browser.widget.displaystyle.DisplayStyleObserver;
+import org.chromium.chrome.browser.widget.displaystyle.UiConfig;
 import org.chromium.chrome.browser.widget.selection.SelectionDelegate.SelectionObserver;
 import org.chromium.ui.UiUtils;
 
@@ -46,7 +48,7 @@ import javax.annotation.Nullable;
  * @param <E> The type of the selectable items this toolbar interacts with.
  */
 public class SelectableListToolbar<E> extends Toolbar implements SelectionObserver<E>,
-        OnClickListener, OnEditorActionListener {
+        OnClickListener, OnEditorActionListener, DisplayStyleObserver {
 
     /**
      * A delegate that handles searching the list of selectable items associated with this toolbar.
@@ -99,6 +101,16 @@ public class SelectableListToolbar<E> extends Toolbar implements SelectionObserv
     private int mSelectionBackgroundColor;
     private int mSearchBackgroundColor;
 
+    private UiConfig mUiConfig;
+    private int mDefaultTitleMarginStartPx;
+    private int mWideDisplayLateralOffsetPx;
+    private int mWideDisplayEndOffsetPx;
+    private int mWideDisplayNavButtonOffsetPx;
+    private int mOriginalContentInsetStart;
+    private int mOriginalContentInsetEnd;
+    private int mOriginalContentInsetStartWithNavigation;
+    private int mOriginalContentInsetEndWithActions;
+
     /**
      * Constructor for inflating from XML.
      */
@@ -376,6 +388,73 @@ public class SelectableListToolbar<E> extends Toolbar implements SelectionObserv
     }
 
     /**
+     * When the toolbar has a wide display style, its contents will be width constrained to
+     * {@link UiConfig#WIDE_DISPLAY_STYLE_MIN_WIDTH_DP}. If the current screen width is greater than
+     * UiConfig#WIDE_DISPLAY_STYLE_MIN_WIDTH_DP, the toolbar contents will be visually centered by
+     * adding padding to both sides.
+     *
+     * @param wideDisplayLateralOffsetPx The offset to use for the lateral padding when in
+     *                                   {@link UiConfig#DISPLAY_STYLE_WIDE}.
+     */
+    public void setHasWideDisplayStyle(int wideDisplayLateralOffsetPx, UiConfig uiConfig) {

[gone, 2017/01/27 18:48:52]

Should this check if it's currently in a wide display style before proceeding? 
I'm worried that it might try to overwrite mOriginalContentInsetStart, et al. if
it's accidentally called again.

[Theresa, 2017/01/27 23:34:46]

It actually could be in wide display style here. I moved the content inset
initialization to onFinishInflate()

+        mWideDisplayLateralOffsetPx = wideDisplayLateralOffsetPx;
+        mDefaultTitleMarginStartPx = getTitleMarginStart();
+        mWideDisplayNavButtonOffsetPx =
+                getResources().getDimensionPixelSize(R.dimen.toolbar_wide_display_nav_icon_offset);
+        mWideDisplayEndOffsetPx = getResources().getDimensionPixelSize(
+                R.dimen.toolbar_wide_display_end_offset);
+
+        mOriginalContentInsetStart = getContentInsetStart();
+        mOriginalContentInsetEnd = getContentInsetEnd();
+        mOriginalContentInsetStartWithNavigation = getContentInsetStartWithNavigation();
+        mOriginalContentInsetEndWithActions = getContentInsetEndWithActions();
+
+        mUiConfig = uiConfig;
+        mUiConfig.addObserver(this);
+
+    }
+
+    @Override
+    public void onDisplayStyleChanged(int newDisplayStyle) {
+        int padding =
+                SelectableListLayout.getPaddingForDisplayStyle(newDisplayStyle, getResources());
+        int paddingStartOffset = 0;
+        int paddingEndOffset = 0;
+        int contentInsetStart = mOriginalContentInsetStart;
+        int contentInsetStartWithNavigation = mOriginalContentInsetStartWithNavigation;
+        int contentInsetEnd = mOriginalContentInsetEnd;
+        int contentInsetEndWithActions = mOriginalContentInsetEndWithActions;
+
+        if (newDisplayStyle == UiConfig.DISPLAY_STYLE_WIDE) {
+            paddingStartOffset = mWideDisplayLateralOffsetPx;
+
+            // The title and nav buttons are inset in the normal display style. In the wide display
+            // style they should be aligned with the starting edge of the list elements.
+            if (mIsSearching || mIsSelectionEnabled) {
+                paddingStartOffset += mWideDisplayNavButtonOffsetPx;
+            } else {
+                paddingStartOffset -= mDefaultTitleMarginStartPx;
+            }
+
+            // The end button is also inset in the normal display. In the wide display it should be
+            // aligned with the ending edge of the list elements.
+            paddingEndOffset = mWideDisplayLateralOffsetPx + mWideDisplayEndOffsetPx;
+
+            contentInsetStart = 0;
+            contentInsetStartWithNavigation = 0;
+            contentInsetEnd = 0;
+            contentInsetEndWithActions = 0;
+        }
+
+        ApiCompatibilityUtils.setPaddingRelative(this,
+                padding + paddingStartOffset, this.getPaddingTop(),
+                padding + paddingEndOffset, this.getPaddingBottom());
+        setContentInsetsRelative(contentInsetStart, contentInsetEnd);
+        setContentInsetStartWithNavigation(contentInsetStartWithNavigation);
+        setContentInsetEndWithActions(contentInsetEndWithActions);
+    }
+
+    /**
      * Set up ActionBarDrawerToggle, a.k.a. hamburger button.
      */
     private void initActionBarDrawerToggle() {
@@ -401,6 +480,8 @@ public class SelectableListToolbar<E> extends Toolbar implements SelectionObserv
 
         mNumberRollView.setVisibility(View.GONE);
         mNumberRollView.setNumber(0, false);
+
+        updateDisplayStyleIfNecessary();
     }
 
     private void showSelectionView(List<E> selectedItems, boolean wasSelectionEnabled) {
@@ -418,6 +499,8 @@ public class SelectableListToolbar<E> extends Toolbar implements SelectionObserv
         mNumberRollView.setNumber(selectedItems.size(), true);
 
         if (mIsSearching) UiUtils.hideKeyboard(mSearchEditText);
+
+        updateDisplayStyleIfNecessary();
     }
 
     private void showSearchViewInternal() {
@@ -427,6 +510,15 @@ public class SelectableListToolbar<E> extends Toolbar implements SelectionObserv
 
         setNavigationButton(NAVIGATION_BUTTON_BACK);
         setBackgroundColor(mSearchBackgroundColor);
+
+        updateDisplayStyleIfNecessary();
+    }
+
+    private void updateDisplayStyleIfNecessary() {
+        if (mUiConfig != null
+                && mUiConfig.getCurrentDisplayStyle() == mUiConfig.DISPLAY_STYLE_WIDE) {

[gone, 2017/01/27 18:48:53]

Any reason why this only calls it if it's wide?  Seems like the function itself
should decide whether anything needs to be done.

[Theresa, 2017/01/27 23:34:46]

onDisplayStyleChanged needs to change things when switching between narrow and
wide display styles or when switching between the selection/normal/search views
in wide display mode. When switching between those views in normal display mode,
calling onDisplayStyleChanged is a no-op. But if it's easier to understand, I
don't mind calling it all the time.

+            onDisplayStyleChanged(mUiConfig.getCurrentDisplayStyle());
+        }
     }
 
     @VisibleForTesting