Skip to content
This repository was archived by the owner on Apr 3, 2020. It is now read-only.

Commit ffcc397

Browse files
author
[email protected]
committed
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 TBR=newt Review URL: https://codereview.chromium.org/1049383008 Cr-Commit-Position: refs/heads/master@{#326642} Review URL: https://codereview.chromium.org/1107663003 Cr-Commit-Position: refs/branch-heads/2357@{#217} Cr-Branched-From: 59d4494-refs/heads/master@{#323860}
1 parent b733fca commit ffcc397

File tree

8 files changed

+88
-34
lines changed

8 files changed

+88
-34
lines changed

‎chrome/android/java/src/org/chromium/chrome/browser/Tab.java

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1584,9 +1584,7 @@ protected void setContentViewCore(ContentViewCore cvc) {
15841584
if (mInfoBarContainer == null) {
15851585
// The InfoBarContainer needs to be created after the ContentView has been natively
15861586
// initialized.
1587-
WebContents webContents = mContentViewCore.getWebContents();
1588-
mInfoBarContainer = new InfoBarContainer(
1589-
mContext, getId(), mContentViewParent, webContents, this);
1587+
mInfoBarContainer = new InfoBarContainer(mContext, getId(), mContentViewParent, this);
15901588
} else {
15911589
mInfoBarContainer.onParentViewChanged(getId(), mContentViewParent);
15921590
}
@@ -2137,8 +2135,9 @@ private long getNativePtr() {
21372135
}
21382136

21392137
/** This is currently called when committing a pre-rendered page. */
2138+
@VisibleForTesting
21402139
@CalledByNative
2141-
private void swapWebContents(
2140+
public void swapWebContents(
21422141
WebContents webContents, boolean didStartLoad, boolean didFinishLoad) {
21432142
ContentViewCore cvc = new ContentViewCore(mContext);
21442143
ContentView cv = ContentView.newInstance(mContext, cvc);
@@ -2199,11 +2198,6 @@ private void setNativePtr(long nativePtr) {
21992198
mNativeTabAndroid = nativePtr;
22002199
}
22012200

2202-
@CalledByNative
2203-
private long getNativeInfoBarContainer() {
2204-
return getInfoBarContainer().getNative();
2205-
}
2206-
22072201
/**
22082202
* @return Whether the TabState representing this Tab has been updated.
22092203
*/

‎chrome/android/java/src/org/chromium/chrome/browser/banners/SwipableOverlayView.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,13 @@ public void setContentViewCore(ContentViewCore contentViewCore) {
205205
}
206206
}
207207

208+
/**
209+
* @return the ContentViewCore that this View is monitoring.
210+
*/
211+
protected ContentViewCore getContentViewCore() {
212+
return mContentViewCore;
213+
}
214+
208215
public void addToParentView(ViewGroup parentView) {
209216
if (parentView != null && parentView.indexOfChild(this) == -1) {
210217
parentView.addView(this, createLayoutParams());

‎chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarContainer.java

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import org.chromium.chrome.browser.Tab;
2121
import org.chromium.chrome.browser.TabObserver;
2222
import org.chromium.chrome.browser.banners.SwipableOverlayView;
23+
import org.chromium.content.browser.ContentViewCore;
2324
import org.chromium.content_public.browser.WebContents;
2425
import org.chromium.ui.base.DeviceFormFactor;
2526

@@ -110,8 +111,7 @@ public InfoBarTransitionInfo(InfoBar bar, View view, int type) {
110111

111112
private Paint mTopBorderPaint;
112113

113-
public InfoBarContainer(
114-
Context context, int tabId, ViewGroup parentView, WebContents webContents, Tab tab) {
114+
public InfoBarContainer(Context context, int tabId, ViewGroup parentView, Tab tab) {
115115
super(context, null);
116116
tab.addObserver(getTabObserver());
117117
setIsSwipable(false);
@@ -144,7 +144,15 @@ public InfoBarContainer(
144144

145145
// Chromium's InfoBarContainer may add an InfoBar immediately during this initialization
146146
// call, so make sure everything in the InfoBarContainer is completely ready beforehand.
147-
mNativeInfoBarContainer = nativeInit(webContents);
147+
mNativeInfoBarContainer = nativeInit();
148+
}
149+
150+
@Override
151+
public void setContentViewCore(ContentViewCore contentViewCore) {
152+
super.setContentViewCore(contentViewCore);
153+
if (getContentViewCore() != null) {
154+
nativeSetWebContents(mNativeInfoBarContainer, contentViewCore.getWebContents());
155+
}
148156
}
149157

150158
@Override
@@ -519,10 +527,6 @@ public static InfoBarContainer childViewOf(ViewGroup parentView) {
519527
return null;
520528
}
521529

522-
public long getNative() {
523-
return mNativeInfoBarContainer;
524-
}
525-
526530
/**
527531
* Sets whether the InfoBarContainer is allowed to auto-hide when the user scrolls the page.
528532
* Expected to be called when Touch Exploration is enabled.
@@ -552,6 +556,8 @@ protected void onViewPressed(MotionEvent event) {
552556
assert false;
553557
}
554558

555-
private native long nativeInit(WebContents webContents);
559+
private native long nativeInit();
560+
private native void nativeSetWebContents(
561+
long nativeInfoBarContainerAndroid, WebContents webContents);
556562
private native void nativeDestroy(long nativeInfoBarContainerAndroid);
557563
}

‎chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,18 +9,22 @@
99
import android.test.suitebuilder.annotation.MediumTest;
1010
import android.test.suitebuilder.annotation.Smoke;
1111

12+
import org.chromium.base.ThreadUtils;
1213
import org.chromium.base.test.util.Feature;
1314
import org.chromium.base.test.util.UrlUtils;
15+
import org.chromium.chrome.browser.ContentViewUtil;
1416
import org.chromium.chrome.browser.location.LocationSettingsTestUtil;
1517
import org.chromium.chrome.shell.ChromeShellTestBase;
1618
import org.chromium.chrome.test.util.InfoBarTestAnimationListener;
1719
import org.chromium.chrome.test.util.InfoBarUtil;
1820
import org.chromium.chrome.test.util.TestHttpServerClient;
1921
import org.chromium.content.browser.test.util.Criteria;
2022
import org.chromium.content.browser.test.util.CriteriaHelper;
23+
import org.chromium.content_public.browser.WebContents;
2124

2225
import java.util.List;
2326

27+
/** Tests for the InfoBars. */
2428
public class InfoBarTest extends ChromeShellTestBase {
2529
private static final long MAX_TIMEOUT = scaleTimeout(2000);
2630
private static final int CHECK_INTERVAL = 500;
@@ -41,6 +45,14 @@ protected void setUp() throws Exception {
4145
super.setUp();
4246

4347
// Register for animation notifications
48+
assertTrue(CriteriaHelper.pollForCriteria(new Criteria() {
49+
@Override
50+
public boolean isSatisfied() {
51+
if (getActivity().getActiveTab() == null) return false;
52+
if (getActivity().getActiveTab().getInfoBarContainer() == null) return false;
53+
return true;
54+
}
55+
}));
4456
InfoBarContainer container = getActivity().getActiveTab().getInfoBarContainer();
4557
mListener = new InfoBarTestAnimationListener();
4658
container.setAnimationListener(mListener);
@@ -128,4 +140,41 @@ public boolean isSatisfied() {
128140
MAX_TIMEOUT, CHECK_INTERVAL);
129141
assertTrue("InfoBar not removed.", mListener.removeInfoBarAnimationFinished());
130142
}
143+
144+
/**
145+
* Verify InfoBarContainers swap the WebContents they are monitoring properly.
146+
*/
147+
@MediumTest
148+
@Feature({"Browser", "Main"})
149+
public void testInfoBarContainerSwapsWebContents() throws InterruptedException {
150+
// Add an infobar.
151+
LocationSettingsTestUtil.setSystemLocationSettingEnabled(true);
152+
loadUrlWithSanitization(TestHttpServerClient.getUrl(GEOLOCATION_PAGE));
153+
assertTrue("InfoBar not added", mListener.addInfoBarAnimationFinished());
154+
List<InfoBar> infoBars = getActivity().getActiveTab().getInfoBarContainer().getInfoBars();
155+
assertEquals("Wrong infobar count", 1, infoBars.size());
156+
157+
// Swap out the WebContents and send the user somewhere so that the InfoBar gets removed.
158+
InfoBarTestAnimationListener removeListener = new InfoBarTestAnimationListener();
159+
getActivity().getActiveTab().getInfoBarContainer().setAnimationListener(removeListener);
160+
ThreadUtils.runOnUiThread(new Runnable() {
161+
@Override
162+
public void run() {
163+
WebContents newContents = ContentViewUtil.createWebContents(false, false);
164+
getActivity().getActiveTab().swapWebContents(newContents, false, false);
165+
}
166+
});
167+
loadUrlWithSanitization(HELLO_WORLD_URL);
168+
assertTrue("InfoBar not removed.", removeListener.removeInfoBarAnimationFinished());
169+
infoBars = getActivity().getActiveTab().getInfoBarContainer().getInfoBars();
170+
assertEquals("Wrong infobar count", 0, infoBars.size());
171+
172+
// Revisiting the original page should make the InfoBar reappear.
173+
InfoBarTestAnimationListener addListener = new InfoBarTestAnimationListener();
174+
getActivity().getActiveTab().getInfoBarContainer().setAnimationListener(addListener);
175+
loadUrlWithSanitization(TestHttpServerClient.getUrl(GEOLOCATION_PAGE));
176+
assertTrue("InfoBar not added", addListener.addInfoBarAnimationFinished());
177+
infoBars = getActivity().getActiveTab().getInfoBarContainer().getInfoBars();
178+
assertEquals("Wrong infobar count", 1, infoBars.size());
179+
}
131180
}

‎chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,11 @@ public Tab createTab(String url, TabLaunchType type) {
175175
public void onToggleFullscreenMode(Tab tab, boolean enable) {
176176
mToolbar.setVisibility(enable ? GONE : VISIBLE);
177177
}
178+
179+
@Override
180+
public void onContentChanged(Tab tab) {
181+
setupContent();
182+
}
178183
});
179184
return tab;
180185
}

‎chrome/browser/android/tab_android.cc

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -294,17 +294,6 @@ void TabAndroid::SwapTabContents(content::WebContents* old_contents,
294294
bool did_start_load,
295295
bool did_finish_load) {
296296
JNIEnv* env = base::android::AttachCurrentThread();
297-
298-
// We need to notify the native InfobarContainer so infobars can be swapped.
299-
InfoBarContainerAndroid* infobar_container =
300-
reinterpret_cast<InfoBarContainerAndroid*>(
301-
Java_Tab_getNativeInfoBarContainer(
302-
env,
303-
weak_java_tab_.get(env).obj()));
304-
InfoBarService* new_infobar_service =
305-
new_contents ? InfoBarService::FromWebContents(new_contents) : NULL;
306-
infobar_container->ChangeInfoBarManager(new_infobar_service);
307-
308297
Java_Tab_swapWebContents(
309298
env,
310299
weak_java_tab_.get(env).obj(),

‎chrome/browser/ui/android/infobars/infobar_container_android.cc

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,14 @@ InfoBarContainerAndroid::~InfoBarContainerAndroid() {
2626
RemoveAllInfoBarsForDestruction();
2727
}
2828

29+
void InfoBarContainerAndroid::SetWebContents(JNIEnv* env,
30+
jobject obj,
31+
jobject web_contents) {
32+
InfoBarService* infobar_service = InfoBarService::FromWebContents(
33+
content::WebContents::FromJavaWebContents(web_contents));
34+
ChangeInfoBarManager(infobar_service);
35+
}
36+
2937
void InfoBarContainerAndroid::Destroy(JNIEnv* env, jobject obj) {
3038
delete this;
3139
}
@@ -74,14 +82,9 @@ void InfoBarContainerAndroid::PlatformSpecificRemoveInfoBar(
7482

7583
// Native JNI methods ---------------------------------------------------------
7684

77-
static jlong Init(JNIEnv* env,
78-
jobject obj,
79-
jobject web_contents) {
85+
static jlong Init(JNIEnv* env, jobject obj) {
8086
InfoBarContainerAndroid* infobar_container =
8187
new InfoBarContainerAndroid(env, obj);
82-
InfoBarService* infobar_service = InfoBarService::FromWebContents(
83-
content::WebContents::FromJavaWebContents(web_contents));
84-
infobar_container->ChangeInfoBarManager(infobar_service);
8588
return reinterpret_cast<intptr_t>(infobar_container);
8689
}
8790

‎chrome/browser/ui/android/infobars/infobar_container_android.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ class InfoBarContainerAndroid : public infobars::InfoBarContainer {
2424
public:
2525
InfoBarContainerAndroid(JNIEnv* env,
2626
jobject infobar_container);
27+
void SetWebContents(JNIEnv* env, jobject obj, jobject web_contents);
2728
void Destroy(JNIEnv* env, jobject obj);
2829

2930
JavaObjectWeakGlobalRef java_container() const {

0 commit comments

Comments
 (0)