Discourage creation of Java infobars without InfoBarDelegates

Discourage creation of Java infobars without InfoBarDelegates

Java:
* Make InfoBarContainer#addInfoBar() a private method so it can't
  be abused anymore.

* Get rid of InfoBar.dismissJavaOnlyInfoBar() and the assorted
  InfoBarListeners to prevent them from being used incorrectly.

* Introduce a SimpleConfirmInfoBarBuilder class that makes it
  easier for devs to create infobars that operate mostly in
  Java that are still backed by native InfoBarDelegates.

C++:
* Expose InfoBarIdentifier enum to Java-side code.  Although the
  end goal is to get rid of Java-only infobars entirely, we're
  still stuck with a generic infobar for popping up errors from
  ChromeWindow#showCallbackNonExistentError() (e.g.).

* Introduce an Android-specific SimpleConfirmInfoBarDelegate
  class that listens for button clicks and alerts a Java-side
  listener.

Tests:
* Renames InfoBarTest2 to InfoBarContainerTest.  These weren't running
  at all because the test filters look for Java classes ending with
  the word Test, not Test2.

* Deletes a few InfoBarTest2 tests that need to be rewritten for the
  stacking world (on newt@'s agenda).

* Edited tests that directly accessed the InfoBarContainer#addInfoBar()
  to use the new SimpleConfirmInfoBarBuilder instead.

BUG=172427, 543205, 569776
Committed: https://crrev.com/7e344082f74a50857d7c2441d1816f69c55b2ae5
Cr-Commit-Position: refs/heads/master@{#370792}

[gone, 2016-01-16 00:55:20]

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

[gone, 2016-01-16 00:55:21]

pkasting@:
I had to expose the InfoBarIdentifier enum we introduced to Java code.  I also
introduced an Android-only ConfirmInfoBarDelegate subclass to point devs at when
they try to skirt the requirement for native-side InfoBarDelegates (which has
happened multiple times).  Does this class seem reasonable?  Its closest
equivalent is the SimpleAlertInfoBarDelegate (in components/), but that doesn't
allow prompting.

newt@:
A bunch of stuff got moved around the test to account for the new InfoBar that
we're building properly instead of directly calling the InfoBarContainer.  A
couple of tests were deleted for later rewriting, as discussed over chat.  I'm
not terribly happy about sending the icon along a Java -> C++ -> Java pathway
when creating the infobar, but I believe that it's the cleanest way to do it
without introducing yet another specialized *InfoBar.java class.  It'll only
happen once per infobar, anyway, so it shouldn't be too awful.

[Peter Kasting, 2016-01-16 01:00:56]

LGTM

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
File chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc
(right):

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc:36: //
InfoBarDelegate overrides:
Nit: You don't directly subclass this class, so these should be combined with
the ConfirmInfoBarDelegate overrides below.

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc:42: //
ConfirmInfoBarDelegate overrides:
Nit: No need for " overrides"

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc:57: };
Nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc:105: if
(primary_str_.size() > 0) buttons |= BUTTON_OK;
Nit: Chromium style avoids placing even short conditional bodies on the same
line.

Prefer !empty() to "size() > 0".

Here's a way of implementing this that avoids these issues and it shorter to
boot:

  return (primary_str_.empty() ? 0 : BUTTON_OK) |
      (secondary_str_.empty() ? 0 : BUTTON_CANCEL);

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc:129: 
Nit: Too many blank lines

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc:155:
scoped_ptr<ConfirmInfoBarDelegate>(new SimpleConfirmInfoBarDelegate(
Nit: make_scoped_ptr?

[newt (away), 2016-01-19 21:54:44]

Thank you, sir.

Just a few minor comments.

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

https://codereview.chromium.org/1596713003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:225:
public void removeInfoBar(InfoBar infoBar) {
@VisibleForTesting

or make this private and add a removeInfoBarForTesting() method

https://codereview.chromium.org/1596713003/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarContainerTest.java
(right):

https://codereview.chromium.org/1596713003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarContainerTest.java:96:
assertTrue(TextUtils.equals(MESSAGE_TEXT, message.getText()));
the error message will be more useful if you use

  assertEquals(MESSAGE_TEXT, message.getText().toString());

https://codereview.chromium.org/1596713003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarContainerTest.java:97:
getInstrumentation().waitForIdleSync();
why? / why now?

https://codereview.chromium.org/1596713003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarContainerTest.java:148:
assertTrue(TextUtils.equals(MESSAGE_TEXT, message.getText()));
likewise

https://codereview.chromium.org/1596713003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarContainerTest.java:230:
fail("Caught exception");
Why catch this exception (and hide the original exception cause)? Seems better
not to catch it; just let it bubble up and cause the test to fail.

You might need to use ThreadUtils.runOnUiThread() instead of
getInstrumentation().runOnMainSync() to make the exception propagate to from the
UI thread to the test thread.

[gone, 2016-01-19 23:43:02]

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

[gone, 2016-01-19 23:43:03]

Alright, comments hopefully addressed.  Thanks!

* Changed infobar_delegate.cc slightly to account for null JNI string warnings
that were appearing in the log.

* Changed GN files so that the build compiles.  Don't know who a good reviewer
for these would be, but things compile fine.  There's no consistency for what
the target should be named in the GN files; went with a naming scheme that fits
in with a few of the other ones.

Adding isherman@ for histograms.xml and thakis@ for chrome.gyp OWNER.

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
File chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc
(right):

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc:36: //
InfoBarDelegate overrides:
On 2016/01/16 01:00:56, Peter Kasting wrote:
> Nit: You don't directly subclass this class, so these should be combined with
> the ConfirmInfoBarDelegate overrides below.

Done.

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc:42: //
ConfirmInfoBarDelegate overrides:
On 2016/01/16 01:00:56, Peter Kasting wrote:
> Nit: No need for " overrides"

Done.

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc:57: };
On 2016/01/16 01:00:56, Peter Kasting wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN

Argh, I always miss that one when I stay in Java too long.  Thanks!

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc:105: if
(primary_str_.size() > 0) buttons |= BUTTON_OK;
On 2016/01/16 01:00:56, Peter Kasting wrote:
> Nit: Chromium style avoids placing even short conditional bodies on the same
> line.
> 
> Prefer !empty() to "size() > 0".
> 
> Here's a way of implementing this that avoids these issues and it shorter to
> boot:
> 
>   return (primary_str_.empty() ? 0 : BUTTON_OK) |
>       (secondary_str_.empty() ? 0 : BUTTON_CANCEL);

Nice; switched over

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc:129: 
On 2016/01/16 01:00:56, Peter Kasting wrote:
> Nit: Too many blank lines

Done.

https://codereview.chromium.org/1596713003/diff/1/chrome/browser/ui/android/i...
chrome/browser/ui/android/infobars/simple_confirm_infobar_builder.cc:155:
scoped_ptr<ConfirmInfoBarDelegate>(new SimpleConfirmInfoBarDelegate(
On 2016/01/16 01:00:56, Peter Kasting wrote:
> Nit: make_scoped_ptr?

Done.

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

https://codereview.chromium.org/1596713003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:225:
public void removeInfoBar(InfoBar infoBar) {
On 2016/01/19 21:54:43, newt wrote:
> @VisibleForTesting
> 
> or make this private and add a removeInfoBarForTesting() method

I think I actually nuked the test that was using this call.  I'll hide it as
package protected.

https://codereview.chromium.org/1596713003/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarContainerTest.java
(right):

https://codereview.chromium.org/1596713003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarContainerTest.java:96:
assertTrue(TextUtils.equals(MESSAGE_TEXT, message.getText()));
On 2016/01/19 21:54:44, newt wrote:
> the error message will be more useful if you use
> 
>   assertEquals(MESSAGE_TEXT, message.getText().toString());

Done.

https://codereview.chromium.org/1596713003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarContainerTest.java:97:
getInstrumentation().waitForIdleSync();
On 2016/01/19 21:54:44, newt wrote:
> why? / why now?

Held over from the earlier function; check line 71 for the previous patch. 
Deleted it anyway; we shouldn't be using this crutch.

https://codereview.chromium.org/1596713003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarContainerTest.java:148:
assertTrue(TextUtils.equals(MESSAGE_TEXT, message.getText()));
On 2016/01/19 21:54:44, newt wrote:
> likewise

Done.

https://codereview.chromium.org/1596713003/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarContainerTest.java:230:
fail("Caught exception");
On 2016/01/19 21:54:44, newt wrote:
> Why catch this exception (and hide the original exception cause)? Seems better
> not to catch it; just let it bubble up and cause the test to fail.
> 
> You might need to use ThreadUtils.runOnUiThread() instead of
> getInstrumentation().runOnMainSync() to make the exception propagate to from
the
> UI thread to the test thread.

Used runOnUiThreadBlocking; the other tests use that.

[gone, 2016-01-19 23:44:00]

On 2016/01/19 23:43:03, dfalcantara wrote:
> * Changed infobar_delegate.cc slightly to account for null JNI string warnings
> that were appearing in the log.

Er, I meant simple_confirm_infobar_builder.cc.

[newt (away), 2016-01-20 00:21:10]

lgtm

[Ilya Sherman, 2016-01-20 01:26:38]

histograms.xml lgtm

[gone, 2016-01-20 03:51:20]

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

[gone, 2016-01-20 03:51:21]

Really, really adding thakis@chromium.org for chrome.gyp owner.

[Nico, 2016-01-21 19:36:52]

lgtm

[gone, 2016-01-21 19:38:13]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2016-01-21 19:38:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1596713003/#ps80001
(title: "Comments, gn build fix")

[commit-bot: I haz the power, 2016-01-21 19:40:15]

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

[commit-bot: I haz the power, 2016-01-21 19:44:23]

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

[commit-bot: I haz the power, 2016-01-21 19:44:24]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gone, 2016-01-21 20:03:58]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2016-01-21 20:03:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, thakis@chromium.org, isherman@chromium.org,
newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1596713003/#ps100001
(title: "Rebasing")

[commit-bot: I haz the power, 2016-01-21 20:04:44]

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

[commit-bot: I haz the power, 2016-01-21 21:39:11]

Committed patchset #6 (id:100001)

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

Description was changed from

==========
Discourage creation of Java infobars without InfoBarDelegates

Java:
* Make InfoBarContainer#addInfoBar() a private method so it can't
  be abused anymore.

* Get rid of InfoBar.dismissJavaOnlyInfoBar() and the assorted
  InfoBarListeners to prevent them from being used incorrectly.

* Introduce a SimpleConfirmInfoBarBuilder class that makes it
  easier for devs to create infobars that operate mostly in
  Java that are still backed by native InfoBarDelegates.

C++:
* Expose InfoBarIdentifier enum to Java-side code.  Although the
  end goal is to get rid of Java-only infobars entirely, we're
  still stuck with a generic infobar for popping up errors from
  ChromeWindow#showCallbackNonExistentError() (e.g.).

* Introduce an Android-specific SimpleConfirmInfoBarDelegate
  class that listens for button clicks and alerts a Java-side
  listener.

Tests:
* Renames InfoBarTest2 to InfoBarContainerTest.  These weren't running
  at all because the test filters look for Java classes ending with
  the word Test, not Test2.

* Deletes a few InfoBarTest2 tests that need to be rewritten for the
  stacking world (on newt@'s agenda).

* Edited tests that directly accessed the InfoBarContainer#addInfoBar()
  to use the new SimpleConfirmInfoBarBuilder instead.

BUG=172427,543205,569776
==========

to

==========
Discourage creation of Java infobars without InfoBarDelegates

Java:
* Make InfoBarContainer#addInfoBar() a private method so it can't
  be abused anymore.

* Get rid of InfoBar.dismissJavaOnlyInfoBar() and the assorted
  InfoBarListeners to prevent them from being used incorrectly.

* Introduce a SimpleConfirmInfoBarBuilder class that makes it
  easier for devs to create infobars that operate mostly in
  Java that are still backed by native InfoBarDelegates.

C++:
* Expose InfoBarIdentifier enum to Java-side code.  Although the
  end goal is to get rid of Java-only infobars entirely, we're
  still stuck with a generic infobar for popping up errors from
  ChromeWindow#showCallbackNonExistentError() (e.g.).

* Introduce an Android-specific SimpleConfirmInfoBarDelegate
  class that listens for button clicks and alerts a Java-side
  listener.

Tests:
* Renames InfoBarTest2 to InfoBarContainerTest.  These weren't running
  at all because the test filters look for Java classes ending with
  the word Test, not Test2.

* Deletes a few InfoBarTest2 tests that need to be rewritten for the
  stacking world (on newt@'s agenda).

* Edited tests that directly accessed the InfoBarContainer#addInfoBar()
  to use the new SimpleConfirmInfoBarBuilder instead.

BUG=172427,543205,569776

Committed: https://crrev.com/7e344082f74a50857d7c2441d1816f69c55b2ae5
Cr-Commit-Position: refs/heads/master@{#370792}
==========

[commit-bot: I haz the power, 2016-01-21 21:40:29]

Patchset 6 (id:??) landed as
https://crrev.com/7e344082f74a50857d7c2441d1816f69c55b2ae5
Cr-Commit-Position: refs/heads/master@{#370792}