Add support for images with descriptions to infobars

Add support for images with descriptions to infobars

Adds support for an image control that has supporting
text labels, and consolidates TODOs involving the TextView
line spacing behavior.

BUG=543205
Committed: https://crrev.com/258b98b54d30d7f790475cd6a1c2bc154d6a36ec
Cr-Commit-Position: refs/heads/master@{#367669}

[gone, 2015-12-22 20:17:14]

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

[gone, 2015-12-22 20:17:14]

newt@: Is there anything super offensive about this generalization of the toggle
layout?  Trying to figure out what to do about the primary and secondary text
(see updated comments on image #5 on the infobar spec page), but this seems like
a good stopgap until I can generalize the button layout stuff.

rouslan@: I don't picture the API changing that much here; to unblock yourself,
try rebasing on top of this CL and then calling InfoBarControlLayout.addIcon()
to add the control you need.

[please use gerrit instead, 2015-12-22 20:21:30]

On 2015/12/22 20:17:14, dfalcantara wrote:
> rouslan@: I don't picture the API changing that much here; to unblock
yourself,
> try rebasing on top of this CL and then calling InfoBarControlLayout.addIcon()
> to add the control you need.

This looks like it will work for me, thank you!

[please use gerrit instead, 2015-12-29 23:16:06]

I've based https://codereview.chromium.org/1540423004/ on this patch. It works!

[gone, 2015-12-31 00:38:53]

newt@: I uploaded https://codereview.chromium.org/1553813002, which will need to
go in first.  This will have to be rewritten to take make use of the updated
layout algorithm.

[gone, 2016-01-05 01:13:30]

Description was changed from

==========
Add support for images with descriptions to infobars

* Generalizes the toggle control layout to have support for
  primary text and secondary descriptive text.

* Allows hiding the toggle switch.

* Adds TODOs to figure out how to generalize the string
  layout behavior.  May need to do refactor the button layout
  code so that it can be used with two TextViews, at this
  rate.

BUG=543205
==========

to

==========
Add support for images with descriptions to infobars

Adds support for an image control that has supporting
text labels, and consolidates TODOs involving the TextView
line spacing behavior.

BUG=543205
==========

[gone, 2016-01-05 01:19:48]

Updated this CL to use the InfoBarDualControlLayout and rebased on top of it. 
API shouldn't have changed at all, though.

[gone, 2016-01-05 20:57:09]

Hokay, ready to review.  Opted to introduce a new XML file instead of
complicating existing one for the toggle.

[newt (away), 2016-01-05 21:20:54]

https://codereview.chromium.org/1543913002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/infobar_control_icon_with_description.xml
(right):

https://codereview.chromium.org/1543913002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/infobar_control_icon_with_description.xml:31:
android:gravity="center_vertical"
This gravity doesn't do anything since height is wrap_content, right? Same for
the TextView below.

https://codereview.chromium.org/1543913002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java
(right):

https://codereview.chromium.org/1543913002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java:198:
* @param iconResourceId   ID of the drawable to use for the icon, or 0 to hide
the ImageView.
Do we need to allow 0 here? I'd prefer not to, unless there's an actual use
case.

In any case, it'll be strange to read code that says:

   controlLayout.addIcon(0, "some text", "some other text");

addIcon() doesn't seem like the right method name for a method that adds two
text views.

[gone, 2016-01-05 21:43:54]

https://codereview.chromium.org/1543913002/diff/40001/chrome/android/java/res...
File chrome/android/java/res/layout/infobar_control_icon_with_description.xml
(right):

https://codereview.chromium.org/1543913002/diff/40001/chrome/android/java/res...
chrome/android/java/res/layout/infobar_control_icon_with_description.xml:31:
android:gravity="center_vertical"
On 2016/01/05 21:20:53, newt wrote:
> This gravity doesn't do anything since height is wrap_content, right? Same for
> the TextView below.

Done.

https://codereview.chromium.org/1543913002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java
(right):

https://codereview.chromium.org/1543913002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java:198:
* @param iconResourceId   ID of the drawable to use for the icon, or 0 to hide
the ImageView.
On 2016/01/05 21:20:53, newt wrote:
> Do we need to allow 0 here? I'd prefer not to, unless there's an actual use
> case.
> 
> In any case, it'll be strange to read code that says:
> 
>    controlLayout.addIcon(0, "some text", "some other text");
> 
> addIcon() doesn't seem like the right method name for a method that adds two
> text views.

Nope, carryover from a rebase.  Nuked it.

[newt (away), 2016-01-05 21:56:16]

lgtm

[gone, 2016-01-05 22:02:36]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2016-01-05 22:05:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1543913002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1543913002/60001

[commit-bot: I haz the power, 2016-01-05 22:19:44]

Description was changed from

==========
Add support for images with descriptions to infobars

Adds support for an image control that has supporting
text labels, and consolidates TODOs involving the TextView
line spacing behavior.

BUG=543205
==========

to

==========
Add support for images with descriptions to infobars

Adds support for an image control that has supporting
text labels, and consolidates TODOs involving the TextView
line spacing behavior.

BUG=543205
==========

[commit-bot: I haz the power, 2016-01-05 22:19:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-05 22:21:27]

Description was changed from

==========
Add support for images with descriptions to infobars

Adds support for an image control that has supporting
text labels, and consolidates TODOs involving the TextView
line spacing behavior.

BUG=543205
==========

to

==========
Add support for images with descriptions to infobars

Adds support for an image control that has supporting
text labels, and consolidates TODOs involving the TextView
line spacing behavior.

BUG=543205

Committed: https://crrev.com/258b98b54d30d7f790475cd6a1c2bc154d6a36ec
Cr-Commit-Position: refs/heads/master@{#367669}
==========

[commit-bot: I haz the power, 2016-01-05 22:21:28]

Patchset 4 (id:??) landed as
https://crrev.com/258b98b54d30d7f790475cd6a1c2bc154d6a36ec
Cr-Commit-Position: refs/heads/master@{#367669}