Begin adding class for laying out InfoBar controls

Begin adding class for laying out InfoBar controls

* Add a skeleton ViewGroup (extending FrameLayout for now) for
  containing standardized InfoBar controls.  It currently only
  allows a way to create a standardized toggle control obeying
  the new UX spec.

* TranslateInfoBar classes now use the new InfoBarLayout to
  create its "Always translate <blah>" checkbox.  The checkbox
  no longer immediately closes the InfoBar -- it instead waits
  until the user hits "done" or "show original".  Users opting
  to ignore the InfoBar or closing it by hitting the "X" don't
  trigger this save.

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

[gone, 2015-11-04 02:05:31]

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

[newt (away), 2015-11-05 05:52:16]

https://codereview.chromium.org/1411853007/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/infobar_subcontrol_toggle.xml (right):

https://codereview.chromium.org/1411853007/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/infobar_subcontrol_toggle.xml:1: <?xml
version="1.0" encoding="utf-8"?>
If this is a "subcontrol" what is a "regular control"? I think we could do away
with the "sub" prefix and just call these "infobar controls". E.g.
infobar_control_toggle.xml, InfoBarControlLayout, etc

https://codereview.chromium.org/1411853007/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/infobar_subcontrol_toggle.xml:16:
android:layout_weight="0"
weight="0" is the default. I'd just leave it out, which is what we typically do.

https://codereview.chromium.org/1411853007/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/infobar_subcontrol_toggle.xml:17:
android:layout_gravity="center_vertical|start"
"start" isn't needed here, and "end" isn't needed in the Switch, since the
TextView takes up all the extra space. (I believe "start" and "end" wouldn't
have any affect anyway, even if the TextView's width were a fixed value.)

https://codereview.chromium.org/1411853007/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/infobar_subcontrol_toggle.xml:28:
android:layout_gravity="center_vertical"
Instead of adding layout_gravity="center_vertical" to all the children, you can
add gravity="center_vertical" to the LinearLayout. Either's fine though.

https://codereview.chromium.org/1411853007/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/infobar_subcontrol_toggle.xml:30:
android:lineSpacingExtra="@dimen/infobar_subcontrol_text_extra_spacing"
I'd recommend lineSpacingMultiplier instead of lineSpacingExtra so that the line
spacing still looks good when the device font size is set to tiny or HUGE.

https://codereview.chromium.org/1411853007/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/infobar_subcontrol_toggle.xml:31:
android:textDirection="locale"
Use textAlignment instead of textDirection. textDirection will force LTR text to
be rendered as RTL, which rearranges the text and makes a general mess of things
if the text is actually LTR. textAlignment merely aligns the text to the left
(resp. right) side of the view, which is what we want.

textDirection is truly problematic when the text comes from the user (e.g. we
used it once on the bookmarks page, which caused rather hilarious problems). But
in general we should avoid it unless it's really needed (as in infobar_text.xml,
where we want to force text like "maps.google.com אָלֶף־בֵּית עִבְרִי
אָלֶף־בֵּית עִבְרִי אָלֶף־בֵּית עִבְרִי" to be displayed as RTL.)

https://codereview.chromium.org/1411853007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarSubLayout.java
(right):

https://codereview.chromium.org/1411853007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarSubLayout.java:51:
if (iconResourceId != -1) {
Android uses 0 and null to signify "no resource". Let's follow that pattern.

Also, you could make the icon visible by default and remove the ImageView if
iconResourceId == 0. That'd be slightly more memory efficient.

https://codereview.chromium.org/1411853007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java
(right):

https://codereview.chromium.org/1411853007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:237:
Switch alwaysSwitch =
This feels hacky. TranslateInfoBar is relying on implementation details of
InfoBarSubLayout.addSwitch(). This method of getting access to switches in
infobars also doesn't seem like it'll work for infobars with multiple switches.

Here are few ideas for fixing this:
 - InfoBarSubLayout.addSwitch() could return the Switch that was created.
 - InfoBarSubLayout.addSwitch() could accept an ID parameter and set the Switch
to have that ID.
 - InfoBarSubLayout.addSwitch() could take a callback that's called when the
switch's value changes.

https://codereview.chromium.org/1411853007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:239:
assert alwaysSwitch != null;
Remove this. The NPE on the next line will notify us very nicely if alwaysSwitch
is null.

[gone, 2015-11-05 20:39:57]

Description was changed from

==========
Begin adding class for laying out InfoBar subcontrols

* Add a skeleton ViewGroup (extending FrameLayout for now) for
  containing standardized InfoBar controls.  It currently only
  allows a way to create a standardized toggle control obeying
  the new UX spec.

* TranslateInfoBar classes now use the new InfoBarSubLayout to
  create its "Always translate <blah>" checkbox.  The checkbox
  no longer immediately closes the InfoBar -- it instead waits
  until the user hits "done" or "show original".  Users opting
  to ignore the InfoBar or closing it by hitting the "X" don't
  trigger this save.

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

to

==========
Begin adding class for laying out InfoBar controls

* Add a skeleton ViewGroup (extending FrameLayout for now) for
  containing standardized InfoBar controls.  It currently only
  allows a way to create a standardized toggle control obeying
  the new UX spec.

* TranslateInfoBar classes now use the new InfoBarSubLayout to
  create its "Always translate <blah>" checkbox.  The checkbox
  no longer immediately closes the InfoBar -- it instead waits
  until the user hits "done" or "show original".  Users opting
  to ignore the InfoBar or closing it by hitting the "X" don't
  trigger this save.

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

[gone, 2015-11-05 20:42:03]

https://chromiumcodereview.appspot.com/1411853007/diff/1/chrome/android/java/...
File chrome/android/java/res/layout/infobar_subcontrol_toggle.xml (right):

https://chromiumcodereview.appspot.com/1411853007/diff/1/chrome/android/java/...
chrome/android/java/res/layout/infobar_subcontrol_toggle.xml:1: <?xml
version="1.0" encoding="utf-8"?>
On 2015/11/05 05:52:15, newt wrote:
> If this is a "subcontrol" what is a "regular control"? I think we could do
away
> with the "sub" prefix and just call these "infobar controls". E.g.
> infobar_control_toggle.xml, InfoBarControlLayout, etc

Done.

https://chromiumcodereview.appspot.com/1411853007/diff/1/chrome/android/java/...
chrome/android/java/res/layout/infobar_subcontrol_toggle.xml:16:
android:layout_weight="0"
On 2015/11/05 05:52:15, newt wrote:
> weight="0" is the default. I'd just leave it out, which is what we typically
do.

Done.

https://chromiumcodereview.appspot.com/1411853007/diff/1/chrome/android/java/...
chrome/android/java/res/layout/infobar_subcontrol_toggle.xml:17:
android:layout_gravity="center_vertical|start"
On 2015/11/05 05:52:15, newt wrote:
> "start" isn't needed here, and "end" isn't needed in the Switch, since the
> TextView takes up all the extra space. (I believe "start" and "end" wouldn't
> have any affect anyway, even if the TextView's width were a fixed value.)

Done.

https://chromiumcodereview.appspot.com/1411853007/diff/1/chrome/android/java/...
chrome/android/java/res/layout/infobar_subcontrol_toggle.xml:28:
android:layout_gravity="center_vertical"
On 2015/11/05 05:52:15, newt wrote:
> Instead of adding layout_gravity="center_vertical" to all the children, you
can
> add gravity="center_vertical" to the LinearLayout. Either's fine though.

Done.

https://chromiumcodereview.appspot.com/1411853007/diff/1/chrome/android/java/...
chrome/android/java/res/layout/infobar_subcontrol_toggle.xml:30:
android:lineSpacingExtra="@dimen/infobar_subcontrol_text_extra_spacing"
On 2015/11/05 05:52:16, newt wrote:
> I'd recommend lineSpacingMultiplier instead of lineSpacingExtra so that the
line
> spacing still looks good when the device font size is set to tiny or HUGE.

Going with 1.4 instead of 1.428, assuming dp=sp and 20/14 is the ratio the
designers were hoping for.

https://chromiumcodereview.appspot.com/1411853007/diff/1/chrome/android/java/...
chrome/android/java/res/layout/infobar_subcontrol_toggle.xml:31:
android:textDirection="locale"
On 2015/11/05 05:52:16, newt wrote:
> Use textAlignment instead of textDirection. textDirection will force LTR text
to
> be rendered as RTL, which rearranges the text and makes a general mess of
things
> if the text is actually LTR. textAlignment merely aligns the text to the left
> (resp. right) side of the view, which is what we want.
> 
> textDirection is truly problematic when the text comes from the user (e.g. we
> used it once on the bookmarks page, which caused rather hilarious problems).
But
> in general we should avoid it unless it's really needed (as in
infobar_text.xml,
> where we want to force text like "maps.google.com אָלֶף־בֵּית עִבְרִי
> אָלֶף־בֵּית עִבְרִי אָלֶף־בֵּית עִבְרִי" to be displayed as RTL.)

Took it out.  Was copied over from the previous version of this toggle.

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

https://chromiumcodereview.appspot.com/1411853007/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarSubLayout.java:51:
if (iconResourceId != -1) {
On 2015/11/05 05:52:16, newt wrote:
> Android uses 0 and null to signify "no resource". Let's follow that pattern.
> 
> Also, you could make the icon visible by default and remove the ImageView if
> iconResourceId == 0. That'd be slightly more memory efficient.

Done.

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

https://chromiumcodereview.appspot.com/1411853007/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:237:
Switch alwaysSwitch =
On 2015/11/05 05:52:16, newt wrote:
> This feels hacky. TranslateInfoBar is relying on implementation details of
> InfoBarSubLayout.addSwitch(). This method of getting access to switches in
> infobars also doesn't seem like it'll work for infobars with multiple
switches.
> 
> Here are few ideas for fixing this:
>  - InfoBarSubLayout.addSwitch() could return the Switch that was created.
>  - InfoBarSubLayout.addSwitch() could accept an ID parameter and set the
Switch
> to have that ID.
>  - InfoBarSubLayout.addSwitch() could take a callback that's called when the
> switch's value changes.

Went with the second option.  Taking in a callback is a little hard with how the
TranslateInfoBar panels work.

https://chromiumcodereview.appspot.com/1411853007/diff/1/chrome/android/java/...
chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateInfoBar.java:239:
assert alwaysSwitch != null;
On 2015/11/05 05:52:16, newt wrote:
> Remove this. The NPE on the next line will notify us very nicely if
alwaysSwitch
> is null.

Done.

[newt (away), 2015-11-05 22:07:44]

lgtm after comments

https://chromiumcodereview.appspot.com/1411853007/diff/20001/chrome/android/j...
File chrome/android/java/res/layout/infobar_control_toggle.xml (right):

https://chromiumcodereview.appspot.com/1411853007/diff/20001/chrome/android/j...
chrome/android/java/res/layout/infobar_control_toggle.xml:15:
android:layout_width="wrap_content"
Don't you need layout_height?

https://chromiumcodereview.appspot.com/1411853007/diff/20001/chrome/android/j...
chrome/android/java/res/layout/infobar_control_toggle.xml:26:
android:layout_gravity="center_vertical"
remove this

https://chromiumcodereview.appspot.com/1411853007/diff/20001/chrome/android/j...
File chrome/android/java/res/values/ids.xml (right):

https://chromiumcodereview.appspot.com/1411853007/diff/20001/chrome/android/j...
chrome/android/java/res/values/ids.xml:14: <item
name="translate_infobar_always_toggle" type="id"/>
alphabetical order?

https://chromiumcodereview.appspot.com/1411853007/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java
(right):

https://chromiumcodereview.appspot.com/1411853007/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java:53:
iconView.setVisibility(View.GONE);
I was saying that you could actually *remove* the view, i.e.
removeView(iconView)

[gone, 2015-11-05 22:21:38]

Oof, sorry.  Fixed things up, hopefully.

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

https://codereview.chromium.org/1411853007/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/infobar_control_toggle.xml:15:
android:layout_width="wrap_content"
On 2015/11/05 22:07:44, newt wrote:
> Don't you need layout_height?

ugh rebasing

https://codereview.chromium.org/1411853007/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/infobar_control_toggle.xml:26:
android:layout_gravity="center_vertical"
On 2015/11/05 22:07:44, newt wrote:
> remove this

Done.

https://codereview.chromium.org/1411853007/diff/20001/chrome/android/java/res...
File chrome/android/java/res/values/ids.xml (right):

https://codereview.chromium.org/1411853007/diff/20001/chrome/android/java/res...
chrome/android/java/res/values/ids.xml:14: <item
name="translate_infobar_always_toggle" type="id"/>
On 2015/11/05 22:07:44, newt wrote:
> alphabetical order?

well, it was the infobar_always_toggle... then I added the word translate.

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

https://codereview.chromium.org/1411853007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java:53:
iconView.setVisibility(View.GONE);
On 2015/11/05 22:07:44, newt wrote:
> I was saying that you could actually *remove* the view, i.e.
> removeView(iconView)

Done.  Was wondering how flipping the condition made any difference.

[gone, 2015-11-05 22:21:50]

Description was changed from

==========
Begin adding class for laying out InfoBar controls

* Add a skeleton ViewGroup (extending FrameLayout for now) for
  containing standardized InfoBar controls.  It currently only
  allows a way to create a standardized toggle control obeying
  the new UX spec.

* TranslateInfoBar classes now use the new InfoBarSubLayout to
  create its "Always translate <blah>" checkbox.  The checkbox
  no longer immediately closes the InfoBar -- it instead waits
  until the user hits "done" or "show original".  Users opting
  to ignore the InfoBar or closing it by hitting the "X" don't
  trigger this save.

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

to

==========
Begin adding class for laying out InfoBar controls

* Add a skeleton ViewGroup (extending FrameLayout for now) for
  containing standardized InfoBar controls.  It currently only
  allows a way to create a standardized toggle control obeying
  the new UX spec.

* TranslateInfoBar classes now use the new InfoBarLayout to
  create its "Always translate <blah>" checkbox.  The checkbox
  no longer immediately closes the InfoBar -- it instead waits
  until the user hits "done" or "show original".  Users opting
  to ignore the InfoBar or closing it by hitting the "X" don't
  trigger this save.

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

[gone, 2015-11-05 22:22:54]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2015-11-05 22:22:54]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1411853007/#ps40001
(title: "Comments")

[newt (away), 2015-11-05 22:23:14]

lgtm

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

https://codereview.chromium.org/1411853007/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java:52:
removeView(iconView);
or switchLayout.removeView()

[commit-bot: I haz the power, 2015-11-05 22:23:47]

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

[commit-bot: I haz the power, 2015-11-05 23:08:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-05 23:09:14]

Patchset 3 (id:??) landed as
https://crrev.com/d05b7a8c01b90fcc8ebb3aeb0df3fc4948b76871
Cr-Commit-Position: refs/heads/master@{#358167}