Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
144283007:
Call WebViewClient#onPageFinished when a main frame fails to load (Closed)
|
Created:
6 years, 10 months ago by hush (inactive) Modified:
6 years, 10 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCall WebViewClient#onPageFinished when a main frame fails to load
This change also reverts https://codereview.chromium.org/133273033/
because you don't need to fire onPageFinished separately for ssl error cancel.
BUG=342859
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251145
Patch Set 1 #Patch Set 2 : added test #
Total comments: 4
Patch Set 3 : address comments #
Total comments: 1
Patch Set 4 : call onReceivedError before onPageFinished #
Total comments: 1
Patch Set 5 : test onReceivedError is called before onPageFinished #
Total comments: 16
Patch Set 6 : address Marcin's comments #Patch Set 7 : fix the test testOnPageFinishedCalledAfterError #
Total comments: 2
Patch Set 8 : address comments about the test #
Messages
Total messages: 22 (0 generated)
Hi, This fix is about b/12837161. Please take a look. Thanks
I will add some instrumentation tests
Hi! I added a test for it. Thank you!
lgtm, but you need an owner's lgtm before submitting. https://codereview.chromium.org/144283007/diff/80001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java (right): https://codereview.chromium.org/144283007/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:88: drop the empty line. https://codereview.chromium.org/144283007/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:121: drop the empty line
removed the empty lines. uploaded a new patch set https://codereview.chromium.org/144283007/diff/80001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java (right): https://codereview.chromium.org/144283007/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:88: On 2014/02/11 22:48:44, sgurun wrote: > drop the empty line. Done. https://codereview.chromium.org/144283007/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:121: On 2014/02/11 22:48:44, sgurun wrote: > drop the empty line Done.
lgtm https://codereview.chromium.org/144283007/diff/150001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/144283007/diff/150001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:70: AwContentsClient.this.onPageFinished(failingUrl); Check what the ordering of onPageFinished vs onReceivedError should be in this case.
On 2014/02/11 23:40:02, boliu wrote: > lgtm > > https://codereview.chromium.org/144283007/diff/150001/android_webview/java/sr... > File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java > (right): > > https://codereview.chromium.org/144283007/diff/150001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:70: > AwContentsClient.this.onPageFinished(failingUrl); > Check what the ordering of onPageFinished vs onReceivedError should be in this > case. I took a look at how the classic webview calls onReceivedError. Whenever there is a main document error, webkit basically calls BrowserFrame#reportError directly through JNI call. The complete call stack is like below: DocumentLoader::mainReceivedError() --> DocumentLoader-->SetMainDocumentError() --> (delegates to frame loader client) FrameLoaderClientAndroid::SetMainDocumentError() --> WebFrame::reportError() Meanwhile, whenever a resource is loaded or an error is received (that is, when FrameLoader::mainReceivedCompleteError() is called), FrameLoader::checkLoadCompleteForThisFrame will be called. And if the main frame is completed, onPageFinished will be called. see: https://cs.corp.google.com/#aosp-jb/external/webkit/Source/WebCore/loader/Doc... We firstly call SetMainDocumentError, (which eventually calls onReceivedError()), then call mainReceivedCompleteError() (which may eventually call onPageFinished, if mainframe finishes) So for classic webview, we call onReceivedError before onPageFinished. However, I don't think developers should really depend their apps on these finicky things.
new patch uploaded
https://codereview.chromium.org/144283007/diff/190001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/144283007/diff/190001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:81: AwContentsClient.this.onPageFinished(failingUrl); Definitely add a comment about the ordering. Even better if you can test it.
still lgtm btw
On 2014/02/12 01:07:06, boliu wrote: > https://codereview.chromium.org/144283007/diff/190001/android_webview/java/sr... > File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java > (right): > > https://codereview.chromium.org/144283007/diff/190001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:81: > AwContentsClient.this.onPageFinished(failingUrl); > Definitely add a comment about the ordering. Even better if you can test it. Done
Hi, I just added a test for the ordering of onPageFinished vs onReceivedError
lgtm
https://codereview.chromium.org/144283007/diff/220001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/144283007/diff/220001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:83: AwContentsClient.this.onPageFinished(failingUrl); The reason we have the errorCode check in the first place is from the "throttle based intercepting" implementation times. Cancelling (AKA intercepting) a request using a throttle generates a net::ERR_ABORTED error. Doing it this way means we'll need to figure out another way when we switch implementations. For now I guess this is fine, might be worth a comment. https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java (right): https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:76: * If url1 is redirected url2, and url2 load is overridden, onPageFinished should still be 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). https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:91: // If you visit redirectUrl, you will be redirected to redirectTargetUrl IMHO this is pretty obvious from the code and doesn't need a comment. https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:98: public boolean shouldOverrideUrlLoading(String url) { 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. https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:99: if (url.equals(redirectTargetUrl)) { I don't think you need this check - we shouldn't call shouldOverride...Loading for the URL you pass to loadUrl. https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:115: assertEquals(1, onPageFinishedHelper.getCallCount()); 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. https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:135: // Make sure onReceivedError is not called before onPageFinished is this really how Classic operated? sigh.. https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:160: assertEquals(invalidUrl, onReceivedErrorHelper.getFailingUrl()); 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.
Hi Marcin, Thanks for the comments! I just uploaded a new patch set. https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java (right): https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:76: * If url1 is redirected url2, and url2 load is overridden, onPageFinished should still be 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. https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:91: // If you visit redirectUrl, you will be redirected to redirectTargetUrl On 2014/02/12 16:03:38, mkosiba wrote: > IMHO this is pretty obvious from the code and doesn't need a comment. Done. https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:98: public boolean shouldOverrideUrlLoading(String url) { 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 https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:99: if (url.equals(redirectTargetUrl)) { 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 https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:115: assertEquals(1, onPageFinishedHelper.getCallCount()); 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. https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:135: // Make sure onReceivedError is not called before onPageFinished 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 https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:160: assertEquals(invalidUrl, onReceivedErrorHelper.getFailingUrl()); 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
Hi! I just fixed testOnPageFinishedCalledAfterError so that you will fail if you call onPageFinished before onReceivedError https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java (right): https://codereview.chromium.org/144283007/diff/220001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:135: // Make sure onReceivedError is not called before onPageFinished 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
lgtm https://codereview.chromium.org/144283007/diff/400001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java (right): https://codereview.chromium.org/144283007/diff/400001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:69: super.onPageFinished(url); maybe assert onReceivedError was called? https://codereview.chromium.org/144283007/diff/400001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:78: assertEquals(0, onReceivedErrorHelper.getCallCount()); I don't think this is needed.
The CQ bit was checked by hush@chromium.org
On 2014/02/13 17:21:49, mkosiba wrote: > lgtm > > https://codereview.chromium.org/144283007/diff/400001/android_webview/javates... > File > android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java > (right): > > https://codereview.chromium.org/144283007/diff/400001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:69: > super.onPageFinished(url); > maybe assert onReceivedError was called? > > https://codereview.chromium.org/144283007/diff/400001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:78: > assertEquals(0, onReceivedErrorHelper.getCallCount()); > I don't think this is needed. Done
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/144283007/450001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hush@chromium.org/144283007/450001
Message was sent while issue was closed.
Change committed as 251145 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
