Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl

Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl

Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl to AwContentsClientBridge. This is a continuation of the work started in
https://codereview.chromium.org/2425423004/ and is the third patch.

BUG=645983
Committed: https://crrev.com/f199141ef70d76697647830f498b25706c0b0d0a
Cr-Commit-Position: refs/heads/master@{#437688}

[sgurun-gerrit only, 2016-12-08 00:29:02]

Description was changed from

==========
Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl

Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl
to AwContentsClientBridge. This is a continuation of the work started in
https://codereview.chromium.org/2425423004/ and is the third patch.

BUG=645983
==========

to

==========
Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl

Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl
to AwContentsClientBridge. This is a continuation of the work started in
https://codereview.chromium.org/2425423004/ and is the third patch.

BUG=645983
==========

[sgurun-gerrit only, 2016-12-08 00:29:02]

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

[sgurun-gerrit only, 2016-12-08 00:29:12]

On 2016/12/08 00:29:02, sgurun wrote:
> mailto:sgurun@chromium.org changed reviewers:
> + mailto:boliu@chromium.org

ptal, thanks

[boliu, 2016-12-08 05:48:38]

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
File android_webview/browser/net/aw_network_delegate.cc (right):

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_network_delegate.cc:23: using
android_webview::AwContentsClientBridgeBase;
you could put the anonymous namespace *inside* the android_webview namespace,
then you don't need these

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_network_delegate.cc:98:
base::Owned(web_request.release()), response_headers));
do MakeUnique<AwWebResourceRequest>(request) here, don't need to declare
|web_request|. you have to change OnReceivedHttpErrorOnUiThread to receive
unique_ptr<AwWebResourceRequest> instead though

or you can just stack allocate AwWebResourceRequest, if copying is ok. I see
RunShouldInterceptRequest does stack allocation. Let's make this consistent

(not relevant after above comment, but use base::Passed for unique_ptrs)

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
File android_webview/browser/net/aw_web_resource_request.cc (right):

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_web_resource_request.cc:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_web_resource_request.cc:12: using std::string;
these are never used

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
File android_webview/browser/net/aw_web_resource_request.h (right):

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_web_resource_request.h:19:
AwWebResourceRequest(const net::URLRequest* request);
const&, to avoid confusion that it's taking ownership of request

also explicit

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_web_resource_request.h:20: virtual
~AwWebResourceRequest() {}
why virtual?

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:311:
base::Owned(web_request.release()),
ditto, use MakeUnique

https://codereview.chromium.org/2558223002/diff/40001/android_webview/native/...
File android_webview/native/aw_contents_client_bridge.cc (right):

https://codereview.chromium.org/2558223002/diff/40001/android_webview/native/...
android_webview/native/aw_contents_client_bridge.cc:451:
ScopedJavaLocalRef<jstring> jurl = ConvertUTF8ToJavaString(env, request.url);
this bunch of stuff is duplicated here and above.. maybe should unify these in
AwWebResourceReqeust?

[sgurun-gerrit only, 2016-12-08 21:56:47]

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
File android_webview/browser/net/aw_network_delegate.cc (right):

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_network_delegate.cc:23: using
android_webview::AwContentsClientBridgeBase;
On 2016/12/08 05:48:38, boliu wrote:
> you could put the anonymous namespace *inside* the android_webview namespace,
> then you don't need these

Done.

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_network_delegate.cc:98:
base::Owned(web_request.release()), response_headers));
On 2016/12/08 05:48:38, boliu wrote:
> do MakeUnique<AwWebResourceRequest>(request) here, don't need to declare
> |web_request|. you have to change OnReceivedHttpErrorOnUiThread to receive
> unique_ptr<AwWebResourceRequest> instead though
> 
> or you can just stack allocate AwWebResourceRequest, if copying is ok. I see
> RunShouldInterceptRequest does stack allocation. Let's make this consistent
> 
> (not relevant after above comment, but use base::Passed for unique_ptrs)

Done.

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
File android_webview/browser/net/aw_web_resource_request.cc (right):

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_web_resource_request.cc:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
On 2016/12/08 05:48:38, boliu wrote:
> no (c)

Done.

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_web_resource_request.cc:12: using std::string;
On 2016/12/08 05:48:38, boliu wrote:
> these are never used

Done.

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
File android_webview/browser/net/aw_web_resource_request.h (right):

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_web_resource_request.h:19:
AwWebResourceRequest(const net::URLRequest* request);
On 2016/12/08 05:48:38, boliu wrote:
> const&, to avoid confusion that it's taking ownership of request
> 
> also explicit

Done.

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_web_resource_request.h:20: virtual
~AwWebResourceRequest() {}
On 2016/12/08 05:48:38, boliu wrote:
> why virtual?

just getting used to I guess. Here in a struct it does not have much value (well
unless somebody decides to add a virtual method)

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:311:
base::Owned(web_request.release()),
On 2016/12/08 05:48:38, boliu wrote:
> ditto, use MakeUnique

stack allocated, copied

https://codereview.chromium.org/2558223002/diff/40001/android_webview/native/...
File android_webview/native/aw_contents_client_bridge.cc (right):

https://codereview.chromium.org/2558223002/diff/40001/android_webview/native/...
android_webview/native/aw_contents_client_bridge.cc:451:
ScopedJavaLocalRef<jstring> jurl = ConvertUTF8ToJavaString(env, request.url);
On 2016/12/08 05:48:38, boliu wrote:
> this bunch of stuff is duplicated here and above.. maybe should unify these in
> AwWebResourceReqeust?

used three times actually.

done.

[boliu, 2016-12-08 22:11:56]

wrap description at 72 chars

lgtm % comments

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
File android_webview/browser/net/aw_web_resource_request.h (right):

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_web_resource_request.h:20: virtual
~AwWebResourceRequest() {}
On 2016/12/08 21:56:46, sgurun wrote:
> On 2016/12/08 05:48:38, boliu wrote:
> > why virtual?
> 
> just getting used to I guess. Here in a struct it does not have much value
(well
> unless somebody decides to add a virtual method)

having an virtual destructor implies it's ok to subclass this. which is clearly
not the case, we want to ban subclassing... mark the class final if you need to,
but destructor shouldn't be virtual, I think you don't need to declare a
destructor at all

https://codereview.chromium.org/2558223002/diff/60001/android_webview/browser...
File android_webview/browser/net/aw_network_delegate.cc (right):

https://codereview.chromium.org/2558223002/diff/60001/android_webview/browser...
android_webview/browser/net/aw_network_delegate.cc:31: AwWebResourceRequest
request,
this can be const&

https://codereview.chromium.org/2558223002/diff/60001/android_webview/browser...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/2558223002/diff/60001/android_webview/browser...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:91:
AwWebResourceRequest request,
ditto const&

[sgurun-gerrit only, 2016-12-08 22:43:18]

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
File android_webview/browser/net/aw_web_resource_request.h (right):

https://codereview.chromium.org/2558223002/diff/40001/android_webview/browser...
android_webview/browser/net/aw_web_resource_request.h:20: virtual
~AwWebResourceRequest() {}
On 2016/12/08 22:11:55, boliu wrote:
> On 2016/12/08 21:56:46, sgurun wrote:
> > On 2016/12/08 05:48:38, boliu wrote:
> > > why virtual?
> > 
> > just getting used to I guess. Here in a struct it does not have much value
> (well
> > unless somebody decides to add a virtual method)
> 
> having an virtual destructor implies it's ok to subclass this. which is
clearly
> not the case, we want to ban subclassing... mark the class final if you need
to,
> but destructor shouldn't be virtual, I think you don't need to declare a
> destructor at all

did not know final. it works best.

https://codereview.chromium.org/2558223002/diff/60001/android_webview/browser...
File android_webview/browser/net/aw_network_delegate.cc (right):

https://codereview.chromium.org/2558223002/diff/60001/android_webview/browser...
android_webview/browser/net/aw_network_delegate.cc:31: AwWebResourceRequest
request,
On 2016/12/08 22:11:56, boliu wrote:
> this can be const&

Done.

https://codereview.chromium.org/2558223002/diff/60001/android_webview/browser...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/2558223002/diff/60001/android_webview/browser...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:91:
AwWebResourceRequest request,
On 2016/12/08 22:11:56, boliu wrote:
> ditto const&

Done here and in aw_contents_io_thread_client

[sgurun-gerrit only, 2016-12-08 22:44:30]

The CQ bit was checked by sgurun@chromium.org

[sgurun-gerrit only, 2016-12-08 22:44:30]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2558223002/#ps80001
(title: "second code review")

[commit-bot: I haz the power, 2016-12-08 22:45:13]

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, 2016-12-08 23:56:01]

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

[commit-bot: I haz the power, 2016-12-08 23:56:02]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[sgurun-gerrit only, 2016-12-08 23:59:55]

On 2016/12/08 23:56:02, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

In file included from
../../android_webview/browser/aw_contents_client_bridge_base.h:10:
../../android_webview/browser/net/aw_web_resource_request.h:21:1: error:
[chromium-style] Complex class/struct needs an explicit out-of-line destructor.
struct AwWebResourceRequest final {
^
../../android_webview/browser/net/aw_web_resource_request.h:25:3: error:
[chromium-style] Complex class/struct needs an explicit out-of-line constructor.
  struct AwJavaWebResourceRequest {
  ^
../../android_webview/browser/net/aw_web_resource_request.h:25:3: error:
[chromium-style] Complex class/struct needs an explicit out-of-line destructor.
3 errors generated.

clang seems to be picky.

[boliu, 2016-12-09 00:01:15]

On 2016/12/08 23:59:55, sgurun wrote:
> On 2016/12/08 23:56:02, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
> 
> In file included from
> ../../android_webview/browser/aw_contents_client_bridge_base.h:10:
> ../../android_webview/browser/net/aw_web_resource_request.h:21:1: error:
> [chromium-style] Complex class/struct needs an explicit out-of-line
destructor.
> struct AwWebResourceRequest final {
> ^
> ../../android_webview/browser/net/aw_web_resource_request.h:25:3: error:
> [chromium-style] Complex class/struct needs an explicit out-of-line
constructor.
>   struct AwJavaWebResourceRequest {
>   ^
> ../../android_webview/browser/net/aw_web_resource_request.h:25:3: error:
> [chromium-style] Complex class/struct needs an explicit out-of-line
destructor.
> 3 errors generated.
> 
> clang seems to be picky.

chromium-specific clang plugin I think

[sgurun-gerrit only, 2016-12-09 20:58:06]

The CQ bit was checked by sgurun@chromium.org

[sgurun-gerrit only, 2016-12-09 20:58:07]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2558223002/#ps120001
(title: "add move assignment")

[commit-bot: I haz the power, 2016-12-09 20:58:51]

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, 2016-12-09 23:26:41]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481317086910000,
"parent_rev": "5766bcaef8d069a69c31a1451b72923070f9b340", "commit_rev":
"fc3e863cf94ca4842264f33c9f7f76bc63fb3e41"}

[commit-bot: I haz the power, 2016-12-09 23:27:05]

Description was changed from

==========
Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl

Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl
to AwContentsClientBridge. This is a continuation of the work started in
https://codereview.chromium.org/2425423004/ and is the third patch.

BUG=645983
==========

to

==========
Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl

Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl
to AwContentsClientBridge. This is a continuation of the work started in
https://codereview.chromium.org/2425423004/ and is the third patch.

BUG=645983

Review-Url: https://codereview.chromium.org/2558223002
==========

[commit-bot: I haz the power, 2016-12-09 23:27:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-12 14:58:14]

Description was changed from

==========
Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl

Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl
to AwContentsClientBridge. This is a continuation of the work started in
https://codereview.chromium.org/2425423004/ and is the third patch.

BUG=645983

Review-Url: https://codereview.chromium.org/2558223002
==========

to

==========
Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl

Move onReceivedError and onReceivedHttpError out of AwContentsIoThreadClientImpl
to AwContentsClientBridge. This is a continuation of the work started in
https://codereview.chromium.org/2425423004/ and is the third patch.

BUG=645983

Committed: https://crrev.com/f199141ef70d76697647830f498b25706c0b0d0a
Cr-Commit-Position: refs/heads/master@{#437688}
==========

[commit-bot: I haz the power, 2016-12-12 14:58:15]

Patchset 7 (id:??) landed as
https://crrev.com/f199141ef70d76697647830f498b25706c0b0d0a
Cr-Commit-Position: refs/heads/master@{#437688}

[boliu, 2016-12-13 16:47:11]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2567043003/ by boliu@chromium.org.

The reason for reverting is: BUG=673736.