android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java - Issue 144283007: Call WebViewClient#onPageFinished when a main frame fails to load

Side by Side Diff: android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java

Issue 144283007: Call WebViewClient#onPageFinished when a main frame fails to load (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: test onReceivedError is called before onPageFinished Created 6 years, 10 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« android_webview/java/src/org/chromium/android_webview/AwContentsClient.java ('K') | « android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java ('k') | no next file » | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 // Copyright 2012 The Chromium Authors. All rights reserved.	1 // Copyright 2012 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 package org.chromium.android_webview.test;	5 package org.chromium.android_webview.test;

6	6

7 import android.test.suitebuilder.annotation.MediumTest;	7 import android.test.suitebuilder.annotation.MediumTest;

8	8

9 import org.chromium.android_webview.AwContents;	9 import org.chromium.android_webview.AwContents;

10 import org.chromium.android_webview.test.util.CommonResources;	10 import org.chromium.android_webview.test.util.CommonResources;

(...skipping 54 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
65 1 /* numberOfCallsToWaitFor */,	65 1 /* numberOfCallsToWaitFor */,

66 WAIT_TIMEOUT_MS,	66 WAIT_TIMEOUT_MS,

67 TimeUnit.MILLISECONDS);	67 TimeUnit.MILLISECONDS);

68 onPageFinishedHelper.waitForCallback(onPageFinishedCallCount,	68 onPageFinishedHelper.waitForCallback(onPageFinishedCallCount,

69 1 /* numberOfCallsToWaitFor */,	69 1 /* numberOfCallsToWaitFor */,

70 WAIT_TIMEOUT_MS,	70 WAIT_TIMEOUT_MS,

71 TimeUnit.MILLISECONDS);	71 TimeUnit.MILLISECONDS);

72 assertEquals(1, onReceivedErrorHelper.getCallCount());	72 assertEquals(1, onReceivedErrorHelper.getCallCount());

73 }	73 }

74	74

	75 /**

	76 * If url1 is redirected url2, and url2 load is overridden, onPageFinished s hould still be
	mkosiba (inactive) 2014/02/12 16:03:38 umm.. this isn't a "proper" JavaDoc, so please don umm.. this isn't a "proper" JavaDoc, so please don't format it as such (drop the 2nd star from the first line and move it into the test body). hush (inactive) 2014/02/12 18:40:40 Done. Show quoted text On 2014/02/12 16:03:38, mkosiba wrote: > umm.. this isn't a "proper" JavaDoc, so please don't format it as such (drop the > 2nd star from the first line and move it into the test body). Done.
	77 * called for url2.

	78 * Steps:

	79 * 1. load url1. url1 onPageStarted

	80 * 2. server redirects url1 to url2. url2 onPageStarted

	81 * 3. shouldOverridedUrlLoading called for url2 and returns true

	82 * 4. url2 onPageFinishedCalled

	83 */

	84 @MediumTest

	85 @Feature({"AndroidWebView"})

	86 public void testOnPageFinishedCalledAfterRedirectedUrlIsOverridden() throws Throwable {

	87 TestWebServer webServer = null;

	88 try {

	89 webServer = new TestWebServer(false);

	90 final String redirectTargetPath = "/redirect_target.html";

	91 // If you visit redirectUrl, you will be redirected to redirectTarge tUrl
	mkosiba (inactive) 2014/02/12 16:03:38 IMHO this is pretty obvious from the code and does IMHO this is pretty obvious from the code and doesn't need a comment. hush (inactive) 2014/02/12 18:40:40 Done. Show quoted text On 2014/02/12 16:03:38, mkosiba wrote: > IMHO this is pretty obvious from the code and doesn't need a comment. Done.
	92 final String redirectTargetUrl = webServer.setResponse(redirectTarge tPath,

	93 "<html><body>hello world</body></html>", null);

	94 final String redirectUrl = webServer.setRedirect("/302.html", redire ctTargetUrl);

	95

	96 mContentsClient = new TestAwContentsClient() {

	97 @Override

	98 public boolean shouldOverrideUrlLoading(String url) {
	mkosiba (inactive) 2014/02/12 16:03:38 I'd prefer either: a) moving the shouldOverrideUr I'd prefer either: a) moving the shouldOverrideUrlLoading implementation and ShouldOverriderUrlLoading CallbackHelper to TestAwContentsClient and reusing that b) writing a common TestContentsClient class at the very top and using that everywhere. This seems like overkill but test cases surprisingly easily accumulate cruft. hush (inactive) 2014/02/12 18:40:40 Sure. Actually I would like to do some refactoring Show quoted text On 2014/02/12 16:03:38, mkosiba wrote: > I'd prefer either: > a) moving the shouldOverrideUrlLoading implementation and > ShouldOverriderUrlLoading CallbackHelper to TestAwContentsClient and reusing > that > b) writing a common TestContentsClient class at the very top and using that > everywhere. > > This seems like overkill but test cases surprisingly easily accumulate cruft. Sure. Actually I would like to do some refactoring in AwContentsClientShouldOverrideUrlLoadingTest.java I want to make use of TestAwContentsClient.ShouldOverrideUrlLoadingHelper which is currently only visible in AwContentsClientShouldOverrideUrlLoadingTest
	99 if (url.equals(redirectTargetUrl)) {
	mkosiba (inactive) 2014/02/12 16:03:38 I don't think you need this check - we shouldn't c I don't think you need this check - we shouldn't call shouldOverride...Loading for the URL you pass to loadUrl. hush (inactive) 2014/02/12 18:40:40 you are right, I don't need this check. I was just Show quoted text On 2014/02/12 16:03:38, mkosiba wrote: > I don't think you need this check - we shouldn't call shouldOverride...Loading > for the URL you pass to loadUrl. you are right, I don't need this check. I was just explicit about what url to override so that the reader understands easily. I will add a comment in the source code instead
	100 return true;

	101 }

	102 return false;

	103 }

	104 };

	105 final AwTestContainerView testContainerView =

	106 createAwTestContainerViewOnMainSync(mContentsClient);

	107 mAwContents = testContainerView.getAwContents();

	108

	109 TestCallbackHelperContainer.OnPageFinishedHelper onPageFinishedHelpe r =

	110 mContentsClient.getOnPageFinishedHelper();

	111

	112 loadUrlSync(mAwContents, onPageFinishedHelper, redirectUrl);

	113

	114 // onPageFinished needs to be called for redirectTargetUrl, but not for redirectUrl

	115 assertEquals(1, onPageFinishedHelper.getCallCount());
	mkosiba (inactive) 2014/02/12 16:03:38 please don't use absolute for this. Grab the curre please don't use absolute for this. Grab the current count before the load and assert it increased by 1. This makes it a lot simpler to move code around later without updating a whole bunch of hardcoded ints. hush (inactive) 2014/02/12 18:40:40 Done. I loadUrlAync then I waitforCallback to incr Show quoted text On 2014/02/12 16:03:38, mkosiba wrote: > please don't use absolute for this. Grab the current count before the load and > assert it increased by 1. This makes it a lot simpler to move code around later > without updating a whole bunch of hardcoded ints. Done. I loadUrlAync then I waitforCallback to increase 1.
	116 assertEquals(redirectTargetUrl, onPageFinishedHelper.getUrl());

	117 } finally {

	118 if (webServer != null) webServer.shutdown();

	119 }

	120 }

	121

	122 /**

	123 * Tests that onPageFinished is called after onReceivedError, if there is in deed an error

	124 */

	125 @MediumTest

	126 @Feature({"AndroidWebView"})

	127 public void testOnPageFinishedCalledAfterOnReceivedError() throws Throwable {

	128 mContentsClient = new TestAwContentsClient() {

	129 private boolean isOnReceivedErrorCalled = false;

	130 private boolean isOnPageFinishedCalled = false;

	131

	132 @Override

	133 public void onReceivedError(int errorCode, String description, Strin g failingUrl) {

	134 isOnReceivedErrorCalled = true;

	135 // Make sure onReceivedError is not called before onPageFinished
	mkosiba (inactive) 2014/02/12 16:03:38 is this really how Classic operated? sigh.. is this really how Classic operated? sigh.. hush (inactive) 2014/02/12 18:40:40 sorry. I typed the wrong comments there. I meant " Show quoted text On 2014/02/12 16:03:38, mkosiba wrote: > is this really how Classic operated? sigh.. sorry. I typed the wrong comments there. I meant "Make sure onReceivedError is called before onPageFinished". That was the classic webview behavior. Actually, I will abandon this test case because there is already a test case that does exactly that, which is testOnPageFinishedCalledAfterError hush (inactive) 2014/02/12 19:41:00 testOnPageFinishedCalledAfterError is not testing Show quoted text On 2014/02/12 18:40:40, hush wrote: > On 2014/02/12 16:03:38, mkosiba wrote: > > is this really how Classic operated? sigh.. > > > sorry. I typed the wrong comments there. I meant "Make sure onReceivedError is > called before onPageFinished". That was the classic webview behavior. > > Actually, I will abandon this test case because there is already a test case > that does exactly that, which is testOnPageFinishedCalledAfterError testOnPageFinishedCalledAfterError is not testing what it is supposed to. You can actually pass the test even if you call onPageFinished before onReceivedError
	136 assertEquals(false, isOnPageFinishedCalled);

	137 super.onReceivedError(errorCode, description, failingUrl);

	138 }

	139

	140 @Override

	141 public void onPageFinished(String url) {

	142 isOnPageFinishedCalled = true;

	143 super.onPageFinished(url);

	144 }

	145 };

	146

	147 final AwTestContainerView testContainerView =

	148 createAwTestContainerViewOnMainSync(mContentsClient);

	149 mAwContents = testContainerView.getAwContents();

	150

	151 TestCallbackHelperContainer.OnPageFinishedHelper onPageFinishedHelper =

	152 mContentsClient.getOnPageFinishedHelper();

	153

	154 TestCallbackHelperContainer.OnReceivedErrorHelper onReceivedErrorHelper =

	155 mContentsClient.getOnReceivedErrorHelper();

	156

	157 final String invalidUrl = "foo://invalid.url.co";

	158 loadUrlSync(mAwContents, onPageFinishedHelper, invalidUrl);

	159

	160 assertEquals(invalidUrl, onReceivedErrorHelper.getFailingUrl());
	mkosiba (inactive) 2014/02/12 16:03:38 a couple of lines above you're checking that onRec a couple of lines above you're checking that onReceivedError is called after onPageFinished but loadUrlSync is only waiting for onPageFinished to be called, not onReceivedError. This means that you might be referencing an uninitialized variable here (actually this means that the test will flake because the CallbackHelper will assert that onReceivedError is called). You should onReceviedErrorHelper.waitForCallback before this line. hush (inactive) 2014/02/12 18:40:40 Sorry. I was actually trying to test onReceivedErr Show quoted text On 2014/02/12 16:03:38, mkosiba wrote: > a couple of lines above you're checking that onReceivedError is called after > onPageFinished but loadUrlSync is only waiting for onPageFinished to be called, > not onReceivedError. This means that you might be referencing an uninitialized > variable here (actually this means that the test will flake because the > CallbackHelper will assert that onReceivedError is called). > You should onReceviedErrorHelper.waitForCallback before this line. Sorry. I was actually trying to test onReceivedError is called before onPageFinished. The assertion in onReceivedError will make sure of that I will abandon this test case because there is already a test case that does exactly that, which is testOnPageFinishedCalledAfterError
	161 assertEquals(invalidUrl, onPageFinishedHelper.getUrl());

	162 }

	163

75 @MediumTest	164 @MediumTest

76 @Feature({"AndroidWebView"})	165 @Feature({"AndroidWebView"})

77 public void testOnPageFinishedNotCalledForValidSubresources() throws Throwab le {	166 public void testOnPageFinishedNotCalledForValidSubresources() throws Throwab le {

78 TestCallbackHelperContainer.OnPageFinishedHelper onPageFinishedHelper =	167 TestCallbackHelperContainer.OnPageFinishedHelper onPageFinishedHelper =

79 mContentsClient.getOnPageFinishedHelper();	168 mContentsClient.getOnPageFinishedHelper();

80	169

81 TestWebServer webServer = null;	170 TestWebServer webServer = null;

82 try {	171 try {

83 webServer = new TestWebServer(false);	172 webServer = new TestWebServer(false);

84	173

(...skipping 121 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
206 executeJavaScriptAndWaitForResult(mAwContents, mContentsClient,	295 executeJavaScriptAndWaitForResult(mAwContents, mContentsClient,

207 "window.history.go(-1)");	296 "window.history.go(-1)");

208	297

209 onPageFinishedHelper.waitForCallback(onPageFinishedCallCount);	298 onPageFinishedHelper.waitForCallback(onPageFinishedCallCount);

210 assertEquals(onPageStartedCallCount, onPageStartedHelper.getCallCoun t());	299 assertEquals(onPageStartedCallCount, onPageStartedHelper.getCallCoun t());

211 } finally {	300 } finally {

212 if (webServer != null) webServer.shutdown();	301 if (webServer != null) webServer.shutdown();

213 }	302 }

214 }	303 }

215 }	304 }

OLD	NEW