[Android History] List item shadows and width constrained styling

[Android History] List item shadows and width constrained styling

BUG=654071
Review-Url: https://codereview.chromium.org/2654193002
Cr-Commit-Position: refs/heads/master@{#447239}
Committed: https://chromium.googlesource.com/chromium/src/+/8422bf10bc4af33a0db2fde5117792ecde362863

[Theresa, 2017-01-25 22:21:30]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 22:22:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-01-25 22:22:29]

twellington@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[Theresa, 2017-01-25 22:22:30]

ptal when convenient - screenshots are in the bug

[commit-bot: I haz the power, 2017-01-26 01:55:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 01:55:55]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Theresa, 2017-01-26 18:04:30]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 18:04:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 18:07:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 18:07:28]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Theresa, 2017-01-27 00:57:35]

Some feedback from UX (so that I don't forget to implement)

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/history_item_view.xml (right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/history_item_view.xml:5: 
From UX review, "The text block is supposed to be 72dp from the left and the
icons 20dp from the left. Looks like current the icons are 24dp from left and
text is 64dp from left"

All other padding/spacing (that's in our control) looks correct now.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java:63:
private boolean mIsSearching;
UX thinks the item view animations should be disabled while searching; I'll
probably handle that as a separate CL.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:180:
private int selectBackgroundResource(boolean isLastInGroup, boolean
isFirstInGroup) {
The background needs to be reselected when other items in the group are removed
so that the top/bottom shadows for the group are displayed correctly.

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

Don't think I saw anything egregious, but I'll take another pass when the
comments are addressed.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/history_date_view.xml (right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/history_date_view.xml:11:
android:layout_marginBottom="15dp"
Little strange to have these be 15 and 17 instead of 16 and 16.  Is it
intentionally off-center?

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java:69:
private final ArrayList<HistoryItemView> mItemViews;
Move this up to the other private final things, then remove the spacing between
the new private int and the other private fields?

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java:317:
}
Pull this and the other version up top out into a function called
getTextDefaultMargin()?

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:181:
if (!isLastInGroup && !isFirstInGroup) return R.drawable.list_item_middle;
might be clearer to just structure it as 

if (blah)
    return thing here
else if (blah2)
    return other thing
else if (blah3)
    return that other thing
else
    return what

alternatively

if (!isLastInGroup) {
    return isFirstInGroup ? R.drawable.list_item_middle :
R.drawable.list_item_top;
} else {
    return isFirstInGroup ? R.drawable.list_item.single :
R.drawable.list_item_bottom;
}

don't know what's cleanest

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java:75:
private final int mDefaultLateralListItemMarginPx;
maybe move these up above mActivity?  seems constant-y.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java:129:
// 4. Initialize empty view.
5.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:69:
public final void setIsFirstOrLastInGroup(boolean isFirst, boolean isLast) {
bit of a mouthful... maybe just add a dedicated setter for each field?

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:289:
* @see #onCreateViewHolder(ViewGroup, int)
See {@link #onCreateViewHolder(ViewGroup, int)}.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/displaystyle/MarginResizer.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/displaystyle/MarginResizer.java:27:
*                            value will be used as the default value for the
lateral margins.
I'd move part of these comments to javadocs for the fields.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListLayout.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListLayout.java:209:
public UiConfig getUiConfig() {
public @Nullable UiConfig getUiConfig() {

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java:399:
public void setHasWideDisplayStyle(int wideDisplayLateralOffsetPx, UiConfig
uiConfig) {
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.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java:519:
&& mUiConfig.getCurrentDisplayStyle() == mUiConfig.DISPLAY_STYLE_WIDE) {
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:33:41]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 23:34:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/history_date_view.xml (right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/history_date_view.xml:11:
android:layout_marginBottom="15dp"
On 2017/01/27 18:48:52, dfalcantara (load balance plz) wrote:
> Little strange to have these be 15 and 17 instead of 16 and 16.  Is it
> intentionally off-center?

Yes, it is intentionally off center. The date view should be closer to the
history items it's associated with than the history items above it.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/history_item_view.xml (right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/history_item_view.xml:5: 
On 2017/01/27 00:57:35, Theresa wrote:
> From UX review, "The text block is supposed to be 72dp from the left and the
> icons 20dp from the left. Looks like current the icons are 24dp from left and
> text is 64dp from left"
> 
> All other padding/spacing (that's in our control) looks correct now.

Done.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/history_item_view.xml:19:
android:paddingEnd="16dp" >
This moved back to the trash can to make its touch target go to the edge of the
screen. When the trash can is GONE (supervised users), padding is
programatically added to this LinearLayout.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java:63:
private boolean mIsSearching;
On 2017/01/27 00:57:35, Theresa wrote:
> UX thinks the item view animations should be disabled while searching; I'll
> probably handle that as a separate CL.

I'm going to do this in a separate CL -- want to make sure UX is happy with how
it looks and change downloads at the same time.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java:69:
private final ArrayList<HistoryItemView> mItemViews;
On 2017/01/27 18:48:52, dfalcantara (load balance plz) wrote:
> Move this up to the other private final things, then remove the spacing
between
> the new private int and the other private fields?

Done.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java:317:
}
On 2017/01/27 18:48:52, dfalcantara (load balance plz) wrote:
> Pull this and the other version up top out into a function called
> getTextDefaultMargin()?

Done.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:180:
private int selectBackgroundResource(boolean isLastInGroup, boolean
isFirstInGroup) {
On 2017/01/27 00:57:35, Theresa wrote:
> The background needs to be reselected when other items in the group are
removed
> so that the top/bottom shadows for the group are displayed correctly.

Done.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:181:
if (!isLastInGroup && !isFirstInGroup) return R.drawable.list_item_middle;
On 2017/01/27 18:48:52, dfalcantara (load balance plz) wrote:
> might be clearer to just structure it as 
> 
> if (blah)
>     return thing here
> else if (blah2)
>     return other thing
> else if (blah3)
>     return that other thing
> else
>     return what
> 
> alternatively
> 
> if (!isLastInGroup) {
>     return isFirstInGroup ? R.drawable.list_item_middle :
> R.drawable.list_item_top;
> } else {
>     return isFirstInGroup ? R.drawable.list_item.single :
> R.drawable.list_item_bottom;
> }
> 
> don't know what's cleanest

Done.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java:75:
private final int mDefaultLateralListItemMarginPx;
On 2017/01/27 18:48:52, dfalcantara (load balance plz) wrote:
> maybe move these up above mActivity?  seems constant-y.

Done.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryManager.java:129:
// 4. Initialize empty view.
On 2017/01/27 18:48:52, dfalcantara (load balance plz) wrote:
> 5.

Done.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:69:
public final void setIsFirstOrLastInGroup(boolean isFirst, boolean isLast) {
On 2017/01/27 18:48:52, dfalcantara (load balance plz) wrote:
> bit of a mouthful... maybe just add a dedicated setter for each field?

Done.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/DateDividedAdapter.java:289:
* @see #onCreateViewHolder(ViewGroup, int)
On 2017/01/27 18:48:52, dfalcantara (load balance plz) wrote:
> See {@link #onCreateViewHolder(ViewGroup, int)}.

Done.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/displaystyle/MarginResizer.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/displaystyle/MarginResizer.java:27:
*                            value will be used as the default value for the
lateral margins.
On 2017/01/27 18:48:52, dfalcantara (load balance plz) wrote:
> I'd move part of these comments to javadocs for the fields.

Done.

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListLayout.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListLayout.java:209:
public UiConfig getUiConfig() {
On 2017/01/27 18:48:52, dfalcantara (load balance plz) wrote:
> public @Nullable UiConfig getUiConfig() {

'@Nullable' annotation modifier does not precede non-annotation modifiers.

I put it above the method:

@Nullable
public UiConfig getUiConfig()


is that okay?

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java:399:
public void setHasWideDisplayStyle(int wideDisplayLateralOffsetPx, UiConfig
uiConfig) {
On 2017/01/27 18:48:52, dfalcantara (load balance plz) wrote:
> 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.

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

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java:519:
&& mUiConfig.getCurrentDisplayStyle() == mUiConfig.DISPLAY_STYLE_WIDE) {
On 2017/01/27 18:48:53, dfalcantara (load balance plz) wrote:
> Any reason why this only calls it if it's wide?  Seems like the function
itself
> should decide whether anything needs to be done.

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.

[commit-bot: I haz the power, 2017-01-28 02:58:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-28 02:58:46]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Theresa, 2017-01-30 16:48:25]

twellington@chromium.org changed reviewers:
+ dgn@chromium.org

[Theresa, 2017-01-30 16:48:26]

+dgn for ntp changes and widget/displaystyle/*

Is peconn@ a good co-owner for widget/displaystyle/*? This package consists of
UiConfig, MarginResizer, DisplayStyleObserver, and DisplayStyleObserverAdapter,
all moved from the ntp package.

[dgn, 2017-01-30 18:21:25]

displaystyle and ntp lgtm

for owners you could link the ntp owners, like we do in the suggestions package

"file://chrome/android/java/src/org/chromium/chrome/browser/ntp/OWNERS"

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java
(right):

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:193:
public void setBackgroundResourceForGroupPosition() {
it looks like we have some logic in common between here and our CardViewHolder.
Could we use the same 9-patch files and share a custom view?

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/displaystyle/MarginResizer.java
(right):

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/displaystyle/MarginResizer.java:11:
* Adds lateral margins to the view when the display style is {@link
UiConfig#DISPLAY_STYLE_WIDE}.
nit: tweak the doc be more like "switch between 2 configs" rather than just "add
margins when wide", as this is what the change does?

[gone, 2017-01-30 23:38:23]

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListLayout.java
(right):

https://codereview.chromium.org/2654193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListLayout.java:209:
public UiConfig getUiConfig() {
On 2017/01/27 23:34:46, Theresa wrote:
> On 2017/01/27 18:48:52, dfalcantara (load balance plz) wrote:
> > public @Nullable UiConfig getUiConfig() {
> 
> '@Nullable' annotation modifier does not precede non-annotation modifiers.
> 
> I put it above the method:
> 
> @Nullable
> public UiConfig getUiConfig()
> 
> 
> is that okay?

Acknowledged.  Wasn't sure where it should go.

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java
(right):

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java:352:
private int getDefaultTextMargin(View view) {
Might be better to just pass in the resources here so that it's obvious that the
actual parent isn't important.

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java
(right):

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java:234:
mOriginalContentInsetEndWithActions = getContentInsetEndWithActions();
Now that this happens only once, do you need to worry about landscape/portrait
rotations?

[Theresa, 2017-01-31 00:05:08]

The CQ bit was checked by twellington@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 00:06:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2017-01-31 00:06:42]

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java
(right):

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryAdapter.java:352:
private int getDefaultTextMargin(View view) {
On 2017/01/30 23:38:22, dfalcantara (load balance plz) wrote:
> Might be better to just pass in the resources here so that it's obvious that
the
> actual parent isn't important.

Done.

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java
(right):

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/history/HistoryItemView.java:193:
public void setBackgroundResourceForGroupPosition() {
On 2017/01/30 18:21:25, dgn wrote:
> it looks like we have some logic in common between here and our
CardViewHolder.
> Could we use the same 9-patch files and share a custom view?

While deceptively similar, the background 9-patches are actually different. The
NTP cards have divider lines between them, while the history items do not.
Ideally we would share resources, but I need UX to converge on designs first.
If/when its possible to share, I'll likely create a static helper method
somewhere; this class already extends SelectableItemView so it cannot share a
base class with CardViewHolder.

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/displaystyle/MarginResizer.java
(right):

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/displaystyle/MarginResizer.java:11:
* Adds lateral margins to the view when the display style is {@link
UiConfig#DISPLAY_STYLE_WIDE}.
On 2017/01/30 18:21:25, dgn wrote:
> nit: tweak the doc be more like "switch between 2 configs" rather than just
"add
> margins when wide", as this is what the change does?

Done.

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java
(right):

https://codereview.chromium.org/2654193002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java:234:
mOriginalContentInsetEndWithActions = getContentInsetEndWithActions();
On 2017/01/30 23:38:22, dfalcantara (load balance plz) wrote:
> Now that this happens only once, do you need to worry about landscape/portrait
> rotations?

Possibly, since Android's calculation of insets is complex and very well may
change between landscape and portrait on some devices. There's no good way to
force the landscape to layout into portrait, through (right?). The worst thing
that could happen here is the 'x' button to clear search text and the number
roll are indented slightly less than they might otherwise be. In practice I
haven't seen this issue occur.

I'll file a bug to track using a custom layout instead of the Android toolbar
class for these lists.

[gone, 2017-01-31 00:07:33]

lgtm

[commit-bot: I haz the power, 2017-01-31 01:22:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 01:22:11]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-01-31 15:56:30]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2017-01-31 15:56:31]

The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2654193002/#ps80001
(title: "Changes from dgn@ and dfalcantara@ reviews")

[commit-bot: I haz the power, 2017-01-31 15:56:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 16:04:50]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485878190990040,
"parent_rev": "20fabcc94dbff322e14eb1f9fd07e53bdcfc3285", "commit_rev":
"8422bf10bc4af33a0db2fde5117792ecde362863"}

[commit-bot: I haz the power, 2017-01-31 16:05:28]

Description was changed from

==========
[Android History] List item shadows and width constrained styling

BUG=654071
==========

to

==========
[Android History] List item shadows and width constrained styling

BUG=654071

Review-Url: https://codereview.chromium.org/2654193002
Cr-Commit-Position: refs/heads/master@{#447239}
Committed:
https://chromium.googlesource.com/chromium/src/+/8422bf10bc4af33a0db2fde51177...
==========

[commit-bot: I haz the power, 2017-01-31 16:05:30]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/8422bf10bc4af33a0db2fde51177...