Properly attach InfoBarContainer when it is swapped to a new WebContents

Properly attach InfoBarContainer when it is swapped to a new WebContents

* Moves the swapping of the InfoBarService from TabAndroid::SwapWebContents() to after TabAndroid::InitWebContents() is called from Tab.setContentViewCore().  This is necessary because InitWebContents attaches the InfoBarService to the WebContents; trying to swap before this point means that the InfoBarContainer will be observing nothing.

* Fixes the ChromeShellActivity/Tab to properly update the ContentViewCore when it is swapped.  This fixes the "Distill page" menu item.

* Adds a test to try to prevent this from happening in the future.

BUG=469026
TEST=InfoBarTest.testInfoBarContainerSwapsWebContents

Committed: https://crrev.com/b96bae62466da45e949427e4ea56e1493d1bd47f
Cr-Commit-Position: refs/heads/master@{#326642}

[gone, 2015-04-20 23:31:22]

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

[gone, 2015-04-22 22:18:13]

Redid all the things and added a unit test.  PTAL.

[nyquist, 2015-04-22 22:28:47]

nyquist@chromium.org changed reviewers:
+ nyquist@chromium.org

[nyquist, 2015-04-22 22:28:47]

I know I'm not the main reviewer here, so please wait for that, but this lgtm.
Very helpful test as well!

I'd wait for newt@ for another look though.

[newt (away), 2015-04-23 04:21:00]

Looks good. I'm mainly just wondering if we can clean up InfoBarContainer's
interface a bit more.

https://codereview.chromium.org/1049383008/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1049383008/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1610:
mInfoBarContainer.setContentViewCore(mContentViewCore);
I think this could be cleaned up a bit more. Food for thought:
 - Does mInfoBarContainer.setContentViewCore() need to be called if we just
constructed mInfoBarContainer?
 - Could InfoBarContainer.nativeSwapWebContents() happen during
mInfoBarContainer.setContentViewCore() instead of during
mInfoBarContainer.onContentViewCoreChanged()?
 - It's somewhat confusing to call mInfoBarContainer.onContentViewCoreChanged()
immediately followed by mInfoBarContainer.setContentViewCore(). Should these
methods be renamed or combined or their responsibilities rejiggered?

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

https://codereview.chromium.org/1049383008/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:523:
public long getNative() {
can this be deleted now?

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

https://codereview.chromium.org/1049383008/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:152:
loadUrlWithSanitization(TestHttpServerClient.getUrl(GEOLOCATION_PAGE));
why "WithSanitization"?

https://codereview.chromium.org/1049383008/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:158:
InfoBarTestAnimationListener removeListener =  new
InfoBarTestAnimationListener();
double space

https://codereview.chromium.org/1049383008/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:170:
assertTrue("Wrong infobar count", infoBars.isEmpty());
assertEquals(..., 0, infoBars.size()) would be slightly more helpful if this
ever fails

[gone, 2015-04-23 18:15:01]

https://codereview.chromium.org/1049383008/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/1049383008/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1610:
mInfoBarContainer.setContentViewCore(mContentViewCore);
>  - Does mInfoBarContainer.setContentViewCore() need to be called if we just
> constructed mInfoBarContainer?
Yeah, the constructor doesn't take in the core.

>  - Could InfoBarContainer.nativeSwapWebContents() happen during
> mInfoBarContainer.setContentViewCore() instead of during
> mInfoBarContainer.onContentViewCoreChanged()?
Not ideal because the constructor of InfoBarContainer says that the native
pointer has to be initialized immediately, but calling swapWebContents
immediately afterward would likely satisfy that requirement.

>  - It's somewhat confusing to call
mInfoBarContainer.onContentViewCoreChanged()
> immediately followed by mInfoBarContainer.setContentViewCore(). Should these
> methods be renamed or combined or their responsibilities rejiggered?
I've rejiggered things again.  Little more invasive than I would have liked, but
it's probably cleaner.

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

https://codereview.chromium.org/1049383008/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java:523:
public long getNative() {
On 2015/04/23 04:20:59, newt wrote:
> can this be deleted now?

Done.

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

https://codereview.chromium.org/1049383008/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:152:
loadUrlWithSanitization(TestHttpServerClient.getUrl(GEOLOCATION_PAGE));
On 2015/04/23 04:21:00, newt wrote:
> why "WithSanitization"?

Test above did it, just copied it downward.

https://codereview.chromium.org/1049383008/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:158:
InfoBarTestAnimationListener removeListener =  new
InfoBarTestAnimationListener();
On 2015/04/23 04:21:00, newt wrote:
> double space

Done.

https://codereview.chromium.org/1049383008/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java:170:
assertTrue("Wrong infobar count", infoBars.isEmpty());
On 2015/04/23 04:20:59, newt wrote:
> assertEquals(..., 0, infoBars.size()) would be slightly more helpful if this
> ever fails

Done.

[newt (away), 2015-04-23 21:01:44]

thanks for the additional improvements :)

lgtm

[gone, 2015-04-23 21:02:18]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2015-04-23 21:02:19]

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

[commit-bot: I haz the power, 2015-04-23 21:03:37]

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

[commit-bot: I haz the power, 2015-04-23 21:08:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-23 21:09:11]

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