[Android WebView] Remove postOnPageFinished from shouldInterceptRequest.

[Android WebView] Remove postOnPageFinished from shouldInterceptRequest.

We previously added a call to onPageFinished from shouldInterceptRequest
in https://codereview.chromium.org/1364863002/.
This was to avoid failing the test
AwContentsClientShouldInterceptRequestTest#testOnReceivedErrorCallback.
However, the cause of the test failing was that with that CL we no
longer post onPageFinished after receiving a response with an empty data
segment (because WebView no longer received
WebContentsObserver.didFinishLoad() callbacks for loads that resulted in
an empty response).
Since then, onPageFinished is again being posted after receiving an
empty response - this was fixed in
https://codereview.chromium.org/2563423004/

The current CL simply removes the unnecessary (and thus incorrect)
onPageFinished call from shouldInterceptRequest and adds a couple of
tests to ensure the original problem with onPageFinished doesn't occur
again.

See b/32629339 for additional discussions.

BUG=656919
Review-Url: https://codereview.chromium.org/2624443002
Cr-Commit-Position: refs/heads/master@{#442621}
Committed: https://chromium.googlesource.com/chromium/src/+/7363ea0b81a3a51eb1513761ecfee892f667fa64

[gsennton, 2017-01-09 16:12:39]

gsennton@chromium.org changed reviewers:
+ japhet@chromium.org, phajdan.jr@chromium.org, sgurun@chromium.org

[gsennton, 2017-01-09 16:12:40]

phajdan.jr@chromium.org: Please review changes in net/test/

sgurun@chromium.org: Please review changes in android_webview/

japhet@ this is just FYI.

Note that I added 3 new tests but only testCalledOnEmptyResponse was failing
before https://codereview.chromium.org/2563423004/, the other two are added to
increase our coverage for the onPageFinished callback.

[Nate Chapin, 2017-01-09 18:20:25]

On 2017/01/09 16:12:40, gsennton wrote:
> mailto:phajdan.jr@chromium.org: Please review changes in net/test/
> 
> mailto:sgurun@chromium.org: Please review changes in android_webview/
> 
> japhet@ this is just FYI.
> 
> Note that I added 3 new tests but only testCalledOnEmptyResponse was failing
> before https://codereview.chromium.org/2563423004/, the other two are added to
> increase our coverage for the onPageFinished callback.

Awesome! Glad this ended up being straightforward.

[sgurun-gerrit only, 2017-01-10 04:59:08]

lgtm with nits

just to make sure: you actually tested this with the said bug right?

https://codereview.chromium.org/2624443002/diff/1/android_webview/javatests/s...
File
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java
(right):

https://codereview.chromium.org/2624443002/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:478:
// Ensure onPageFinished isn't called several times. By loading next page.
fix the comment

https://codereview.chromium.org/2624443002/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:495:
* Ensure onPageFinished is called when a provisional load is cancelled.
nit properly update copy/paste from above.

[gsennton, 2017-01-10 16:08:24]

https://codereview.chromium.org/2624443002/diff/1/android_webview/javatests/s...
File
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java
(right):

https://codereview.chromium.org/2624443002/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:478:
// Ensure onPageFinished isn't called several times. By loading next page.
On 2017/01/10 04:59:08, sgurun wrote:
> fix the comment 

Done.

https://codereview.chromium.org/2624443002/diff/1/android_webview/javatests/s...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnPageFinishedTest.java:495:
* Ensure onPageFinished is called when a provisional load is cancelled.
On 2017/01/10 04:59:08, sgurun wrote:
> nit properly update copy/paste from above.

Done.

[Paweł Hajdan Jr., 2017-01-10 16:26:29]

LGTM

[gsennton, 2017-01-10 16:53:37]

The CQ bit was checked by gsennton@chromium.org

[gsennton, 2017-01-10 16:53:38]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org
Link to the patchset: https://codereview.chromium.org/2624443002/#ps20001
(title: "Fix Selim's nits (comment fixes).")

[commit-bot: I haz the power, 2017-01-10 16:53:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 17:29:35]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484067217968080,
"parent_rev": "b9147616bf449986d216f2a67f1bd48924c793a7", "commit_rev":
"7363ea0b81a3a51eb1513761ecfee892f667fa64"}

[commit-bot: I haz the power, 2017-01-10 17:30:33]

Description was changed from

==========
[Android WebView] Remove postOnPageFinished from shouldInterceptRequest.

We previously added a call to onPageFinished from shouldInterceptRequest
in https://codereview.chromium.org/1364863002/.
This was to avoid failing the test
AwContentsClientShouldInterceptRequestTest#testOnReceivedErrorCallback.
However, the cause of the test failing was that with that CL we no
longer post onPageFinished after receiving a response with an empty data
segment (because WebView no longer received
WebContentsObserver.didFinishLoad() callbacks for loads that resulted in
an empty response).
Since then, onPageFinished is again being posted after receiving an
empty response - this was fixed in
https://codereview.chromium.org/2563423004/

The current CL simply removes the unnecessary (and thus incorrect)
onPageFinished call from shouldInterceptRequest and adds a couple of
tests to ensure the original problem with onPageFinished doesn't occur
again.

See b/32629339 for additional discussions.

BUG=656919
==========

to

==========
[Android WebView] Remove postOnPageFinished from shouldInterceptRequest.

We previously added a call to onPageFinished from shouldInterceptRequest
in https://codereview.chromium.org/1364863002/.
This was to avoid failing the test
AwContentsClientShouldInterceptRequestTest#testOnReceivedErrorCallback.
However, the cause of the test failing was that with that CL we no
longer post onPageFinished after receiving a response with an empty data
segment (because WebView no longer received
WebContentsObserver.didFinishLoad() callbacks for loads that resulted in
an empty response).
Since then, onPageFinished is again being posted after receiving an
empty response - this was fixed in
https://codereview.chromium.org/2563423004/

The current CL simply removes the unnecessary (and thus incorrect)
onPageFinished call from shouldInterceptRequest and adds a couple of
tests to ensure the original problem with onPageFinished doesn't occur
again.

See b/32629339 for additional discussions.

BUG=656919

Review-Url: https://codereview.chromium.org/2624443002
Cr-Commit-Position: refs/heads/master@{#442621}
Committed:
https://chromium.googlesource.com/chromium/src/+/7363ea0b81a3a51eb1513761ecfe...
==========

[commit-bot: I haz the power, 2017-01-10 17:30:35]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/7363ea0b81a3a51eb1513761ecfe...