Visual makeover for infobars.

Visual makeover for infobars.

NOTRY=true
BUG=375379
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282206

[newt (away), 2014-07-08 22:31:28]

PTAL

[gone, 2014-07-09 21:13:46]

TAL.

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java
(right):

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:224:
View tertiaryButton = mContentView.findViewById(R.id.button_tertiary);
Wonder if it'd make sense to just do a loop on all the Button children.

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java
(right):

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:27:
* - (optional) An icon representing the infobar's purpose on the left side.
nit: Maybe move this below the required ones?

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:89:
void setVerticalMode(int verticalSpacing, int topMargin, int bottomMargin) {
do you ever pass in a non-zero topMargin?

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:207:
*  - Side by side on a separate row, each taking up half the width of the
infobar, OR
nit: remove the OR, reword the "will be displayed" bit?

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:210:
public void setCustomContent(View view1, View view2) {
Only two views because mocks have shown only two custom bits max?  (e.g. two
spinners)

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:236:
public void setButtons(String primaryText, String secondaryText, String
tertiaryText) {
public -> JavaDoc

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:269:
tertiaryButton.setTextColor(0xff969696);
pull this out to a color in the XML?

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:469:
int width = 0;
maybe rewrite this so that it early exits for the stacked case?  it's odd to
enter the for loop briefly and then immediately break.

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:528:
// Expand the children to take up the entire row.
nit: consider splitting off that rounding logic, or at least explaining it? 
took me a sec to understand what the +1 / 2 meant.

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/MessageInfoBar.java
(right):

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/MessageInfoBar.java:30:
*/
Really tempted to mark this as @Deprecated.

[newt (away), 2014-07-09 21:54:59]

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java
(right):

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:224:
View tertiaryButton = mContentView.findViewById(R.id.button_tertiary);
On 2014/07/09 21:13:45, dfalcantara wrote:
> Wonder if it'd make sense to just do a loop on all the Button children.

I tried this idea but recursing over all descendant views is non-trivial and a
fair bit more code. I'll leave this as is for now. Maybe once we add a
quaternaryButton, we should reconsider ;)

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java
(right):

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:27:
* - (optional) An icon representing the infobar's purpose on the left side.
On 2014/07/09 21:13:45, dfalcantara wrote:
> nit: Maybe move this below the required ones?

Done.

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:89:
void setVerticalMode(int verticalSpacing, int topMargin, int bottomMargin) {
On 2014/07/09 21:13:45, dfalcantara wrote:
> do you ever pass in a non-zero topMargin?

Nope. Remove it for now. It's easy enough to add back if we need it.

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:207:
*  - Side by side on a separate row, each taking up half the width of the
infobar, OR
On 2014/07/09 21:13:45, dfalcantara wrote:
> nit: remove the OR, reword the "will be displayed" bit?

Done.

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:210:
public void setCustomContent(View view1, View view2) {
On 2014/07/09 21:13:45, dfalcantara wrote:
> Only two views because mocks have shown only two custom bits max?  (e.g. two
> spinners)

Yup. It didn't seem worth trying to generalize this to more views, since a) we
may never have more views, and b) there's no reason to assume that a
hypothetical infobar with 3 side-by-side views would share the same layout logic
anyway.

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:236:
public void setButtons(String primaryText, String secondaryText, String
tertiaryText) {
On 2014/07/09 21:13:45, dfalcantara wrote:
> public -> JavaDoc

Done.

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:269:
tertiaryButton.setTextColor(0xff969696);
On 2014/07/09 21:13:45, dfalcantara wrote:
> pull this out to a color in the XML?

Done.

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:469:
int width = 0;
On 2014/07/09 21:13:45, dfalcantara wrote:
> maybe rewrite this so that it early exits for the stacked case?  it's odd to
> enter the for loop briefly and then immediately break.

Done.

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:528:
// Expand the children to take up the entire row.
On 2014/07/09 21:13:45, dfalcantara wrote:
> nit: consider splitting off that rounding logic, or at least explaining it? 
> took me a sec to understand what the +1 / 2 meant.

Gotta use every last pixel!

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/MessageInfoBar.java
(right):

https://chromiumcodereview.appspot.com/378043002/diff/1/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/infobar/MessageInfoBar.java:30:
*/
On 2014/07/09 21:13:45, dfalcantara wrote:
> Really tempted to mark this as @Deprecated.

Acknowledged.

[gone, 2014-07-09 21:58:08]

One last question, lgtm if it can't happen.

https://chromiumcodereview.appspot.com/378043002/diff/20001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java
(right):

https://chromiumcodereview.appspot.com/378043002/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:474:
if (group.isStacked) return getWidthWithMargins(group.views[0]);
safe to assume that there's always at least one view?

[newt (away), 2014-07-09 22:07:18]

https://chromiumcodereview.appspot.com/378043002/diff/20001/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java
(right):

https://chromiumcodereview.appspot.com/378043002/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarLayout.java:474:
if (group.isStacked) return getWidthWithMargins(group.views[0]);
On 2014/07/09 21:58:08, dfalcantara wrote:
> safe to assume that there's always at least one view?

Yea. addGroup() is a private method and it's always called with at least one
parameter.

[newt (away), 2014-07-09 22:15:51]

+dtrainor for magical OWNERS goodness

[David Trainor- moved to gerrit, 2014-07-09 22:23:41]

rubber stamp lgtm.

[newt (away), 2014-07-09 22:26:04]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-07-09 22:28:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/newt@chromium.org/378043002/20001

[newt (away), 2014-07-09 22:31:29]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-07-09 22:31:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/newt@chromium.org/378043002/40001

[newt (away), 2014-07-09 22:59:42]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-07-09 23:01:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/newt@chromium.org/378043002/60001

[newt (away), 2014-07-10 01:06:54]

The CQ bit was unchecked by newt@chromium.org

[newt (away), 2014-07-10 01:07:03]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-07-10 01:08:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/newt@chromium.org/378043002/60001

[commit-bot: I haz the power, 2014-07-10 01:12:02]

Change committed as 282206