[Android WebView] Implement OnReceivedHttpError callback

[Android WebView] Implement OnReceivedHttpError callback

Implement OnReceivedHttpError via NetworkDelegate::OnHeadersReceived
for status codes >= 400.

BUG=456782
Committed: https://crrev.com/8a4a31d0494d4b19d9c5cf60da890106ea1da518
Cr-Commit-Position: refs/heads/master@{#319856}

[mnaganov (inactive), 2015-03-05 17:06:44]

mnaganov@chromium.org changed reviewers:
+ sgurun@chromium.org

[sgurun-gerrit only, 2015-03-06 03:03:07]

On 2015/03/05 17:06:45, mnaganov (cr) wrote:

I am going to look at the tests tomorrow but a question on general approach.
seems we are bubbling all http headers to java only to throw them away if there
is no error? Why can't we do it native? isn't it a lot of extra copying.

[sgurun-gerrit only, 2015-03-06 06:49:26]

a few more questions. Thanks! there will be a follow up cl for the
onreceivederror callback I believe.

https://codereview.chromium.org/976393002/diff/1/android_webview/glue/java/sr...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java
(right):

https://codereview.chromium.org/976393002/diff/1/android_webview/glue/java/sr...
android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:596:
@Override
this code will need to go to the "new" glue layer.

https://codereview.chromium.org/976393002/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedHttpErrorTest.java
(right):

https://codereview.chromium.org/976393002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedHttpErrorTest.java:28:
@MinAndroidSdkLevel(Build.VERSION_CODES.KITKAT)
is this correct (and actually useful)? maybe put a note to fix it after next API
level bump.

https://codereview.chromium.org/976393002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedHttpErrorTest.java:130:
assertTrue(response.getResponseHeaders().size() > 0);
himm, question why do you only check the header size >0 here but not verify the
headers themselves like what you did in test above?

[mnaganov (inactive), 2015-03-06 12:58:25]

> I am going to look at the tests tomorrow but a question on general approach.
> seems we are bubbling all http headers to java only to throw them away if
there
> is no error? Why can't we do it native? isn't it a lot of extra copying.

Agreed. Initially I though we may use this callback in future for something
else, but actually let's wait until such a need actually comes up. I changed
to filtering out non-error codes as early as possible.

> a few more questions. Thanks! there will be a follow up cl for the
> onreceivederror callback I believe.

Of course, the changes are too big to squash them together.

https://codereview.chromium.org/976393002/diff/1/android_webview/glue/java/sr...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java
(right):

https://codereview.chromium.org/976393002/diff/1/android_webview/glue/java/sr...
android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:596:
@Override
On 2015/03/06 06:49:25, sgurun wrote:
> this code will need to go to the "new" glue layer.

I would argue that glue code functions should still receive Aw.. types, not
android.webkit types, because otherwise how will you do conversion of arguments
into types that are not in android.webkit yet? Conversion from internal types to
API types is exactly the task for the glue code.

https://codereview.chromium.org/976393002/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedHttpErrorTest.java
(right):

https://codereview.chromium.org/976393002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedHttpErrorTest.java:28:
@MinAndroidSdkLevel(Build.VERSION_CODES.KITKAT)
On 2015/03/06 06:49:26, sgurun wrote:
> is this correct (and actually useful)? maybe put a note to fix it after next
API
> level bump.

Absolutely useful. This is to prevent our tests from running on pre-KitKat
devices from Chromium FYI waterfall. It's not for restricting these test from
running on upcoming Android versions.

https://codereview.chromium.org/976393002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedHttpErrorTest.java:130:
assertTrue(response.getResponseHeaders().size() > 0);
On 2015/03/06 06:49:26, sgurun wrote:
> himm, question why do you only check the header size >0 here but not verify
the
> headers themselves like what you did in test above?

OK, added testing for "Content-Type" header.

[mnaganov (inactive), 2015-03-06 15:23:31]

On 2015/03/06 12:58:25, mnaganov (cr) wrote:
> > I am going to look at the tests tomorrow but a question on general approach.
> > seems we are bubbling all http headers to java only to throw them away if
> there
> > is no error? Why can't we do it native? isn't it a lot of extra copying.
> 
> Agreed. Initially I though we may use this callback in future for something
> else, but actually let's wait until such a need actually comes up. I changed
> to filtering out non-error codes as early as possible.
> 
> > a few more questions. Thanks! there will be a follow up cl for the
> > onreceivederror callback I believe.
> 
> Of course, the changes are too big to squash them together.
> 
>
https://codereview.chromium.org/976393002/diff/1/android_webview/glue/java/sr...
> File
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java
> (right):
> 
>
https://codereview.chromium.org/976393002/diff/1/android_webview/glue/java/sr...
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewContentsClientAdapter.java:596:
> @Override
> On 2015/03/06 06:49:25, sgurun wrote:
> > this code will need to go to the "new" glue layer.
> 
> I would argue that glue code functions should still receive Aw.. types, not
> android.webkit types, because otherwise how will you do conversion of
arguments
> into types that are not in android.webkit yet? Conversion from internal types
to
> API types is exactly the task for the glue code.
>

On second thought, I believe I misunderstood your comment. Did you actually mean
that you just don't want to see this stub method upstream? To my POV, having it
here
will reduce the number of different lines between upstream and downstream glue.
 
>
https://codereview.chromium.org/976393002/diff/1/android_webview/javatests/sr...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedHttpErrorTest.java
> (right):
> 
>
https://codereview.chromium.org/976393002/diff/1/android_webview/javatests/sr...
>
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedHttpErrorTest.java:28:
> @MinAndroidSdkLevel(Build.VERSION_CODES.KITKAT)
> On 2015/03/06 06:49:26, sgurun wrote:
> > is this correct (and actually useful)? maybe put a note to fix it after next
> API
> > level bump.
> 
> Absolutely useful. This is to prevent our tests from running on pre-KitKat
> devices from Chromium FYI waterfall. It's not for restricting these test from
> running on upcoming Android versions.
> 
>
https://codereview.chromium.org/976393002/diff/1/android_webview/javatests/sr...
>
android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedHttpErrorTest.java:130:
> assertTrue(response.getResponseHeaders().size() > 0);
> On 2015/03/06 06:49:26, sgurun wrote:
> > himm, question why do you only check the header size >0 here but not verify
> the
> > headers themselves like what you did in test above?
> 
> OK, added testing for "Content-Type" header.

[sgurun-gerrit only, 2015-03-06 18:12:36]

LGTM

> 
> On second thought, I believe I misunderstood your comment. Did you actually
mean
> that you just don't want to see this stub method upstream? To my POV, having
it
> here
> will reduce the number of different lines between upstream and downstream
glue.

Yes, this is what I mean, but since it is not very clear how to land glue layer
changes yet, I don't have any strong preference either way.

thanks!

[sgurun-gerrit only, 2015-03-06 18:12:57]

https://codereview.chromium.org/976393002/diff/20001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java
(right):

https://codereview.chromium.org/976393002/diff/20001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java:88:
// Note that we receive un-coalesced response header lines, thus we need to
combine
I think not all headers can be coalesced safely. Maybe not terribly important
since this is for error reporting only at the time but still something to keep
in mind.

[Torne, 2015-03-06 18:16:32]

On 2015/03/06 18:12:36, sgurun wrote:
> LGTM
> 
> > 
> > On second thought, I believe I misunderstood your comment. Did you actually
> mean
> > that you just don't want to see this stub method upstream? To my POV, having
> it
> > here
> > will reduce the number of different lines between upstream and downstream
> glue.
> 
> Yes, this is what I mean, but since it is not very clear how to land glue
layer
> changes yet, I don't have any strong preference either way.
> 
> thanks!

I believe we need the stub upstream, because this is a callback - if it's not
there then the code *outside* the glue layer which calls this function will not
compile, and we are not going to have a downstream copy of *that* code. Does
that make sense, or am I missing something?

[mnaganov (inactive), 2015-03-06 20:45:33]

On 2015/03/06 18:12:57, sgurun wrote:
>
https://codereview.chromium.org/976393002/diff/20001/android_webview/java/src...
> File
>
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java
> (right):
> 
>
https://codereview.chromium.org/976393002/diff/20001/android_webview/java/src...
>
android_webview/java/src/org/chromium/android_webview/AwContentsIoThreadClient.java:88:
> // Note that we receive un-coalesced response header lines, thus we need to
> combine
> I think not all headers can be coalesced safely. Maybe not terribly important
> since this is for error reporting only at the time but still something to keep
> in mind.

Right, there is a note in
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_resp...
that this can break cookies with unquoted commas split across several headers
with the same name.
But since we have chosen to use a (String -> String) map for passing headers,
there is nothing better
we can do here.

[mnaganov (inactive), 2015-03-06 20:52:23]

On 2015/03/06 18:16:32, Torne wrote:
> On 2015/03/06 18:12:36, sgurun wrote:
> > LGTM
> > 
> > > 
> > > On second thought, I believe I misunderstood your comment. Did you
actually
> > mean
> > > that you just don't want to see this stub method upstream? To my POV,
having
> > it
> > > here
> > > will reduce the number of different lines between upstream and downstream
> > glue.
> > 
> > Yes, this is what I mean, but since it is not very clear how to land glue
> layer
> > changes yet, I don't have any strong preference either way.
> > 
> > thanks!
> 
> I believe we need the stub upstream, because this is a callback - if it's not
> there then the code *outside* the glue layer which calls this function will
not
> compile, and we are not going to have a downstream copy of *that* code. Does
> that make sense, or am I missing something?

We could just keep this method of AwContentsClient non-abstract, so
WebViewContentsClientAdapter
would not need to override it with a stub upstream, only downstream. I think we
used this
approach before. But to me, keeping it abstract and having a stub in the glue
leads to
less modifications in future.

[mnaganov (inactive), 2015-03-06 20:52:55]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2015-03-06 20:53:42]

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

[commit-bot: I haz the power, 2015-03-06 21:13:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-06 21:13:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sgurun-gerrit only, 2015-03-06 21:28:42]

On 2015/03/06 21:13:55, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

I think you need an lgtm from net owners.

[mnaganov (inactive), 2015-03-09 09:39:35]

mnaganov@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[mnaganov (inactive), 2015-03-09 09:39:36]

Hi Pawel, can you please take a look at the net/test change?

[Paweł Hajdan Jr., 2015-03-10 09:32:50]

net/test LGTM

[mnaganov (inactive), 2015-03-10 09:33:29]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2015-03-10 09:33:47]

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

[commit-bot: I haz the power, 2015-03-10 10:01:28]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-10 10:02:23]

Patchset 2 (id:??) landed as
https://crrev.com/8a4a31d0494d4b19d9c5cf60da890106ea1da518
Cr-Commit-Position: refs/heads/master@{#319856}