[Android] Make AndroidStreamReaderURLRequestJob to start loading asynchronously.

[Android] Make AndroidStreamReaderURLRequestJob to start loading asynchronously.

This fixes undesired destruction of ResourceLoader inside of StartRequestInternal.

R=mkosiba@chromium.org


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161907

[mkosiba (inactive), 2012-10-15 16:08:55]

lgtm

[commit-bot: I haz the power, 2012-10-15 16:13:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/11147021/1

[joth, 2012-10-15 17:01:46]

https://codereview.chromium.org/11147021/diff/1/android_webview/native/androi...
File android_webview/native/android_stream_reader_url_request_job.h (right):

https://codereview.chromium.org/11147021/diff/1/android_webview/native/androi...
android_webview/native/android_stream_reader_url_request_job.h:78:
base::WeakPtrFactory<AndroidStreamReaderURLRequestJob> weak_factory_;
net::URLRequestJob is refcounted, so it feels like you should just use that to
keep the request alive while the async posttask is in flight, rather than add
weak_ptr semantics into the class too. If there's a specific reason ref-counting
doesn't work, you should add a comment (here, or where the task is posted,
whichever makes more sense).

(If the java-side looses interest and gets GCed I think the jweaks will
automatically turn this into a no-op)

[mnaganov (inactive), 2012-10-15 17:18:25]

https://codereview.chromium.org/11147021/diff/1/android_webview/native/androi...
File android_webview/native/android_stream_reader_url_request_job.h (right):

https://codereview.chromium.org/11147021/diff/1/android_webview/native/androi...
android_webview/native/android_stream_reader_url_request_job.h:78:
base::WeakPtrFactory<AndroidStreamReaderURLRequestJob> weak_factory_;
On 2012/10/15 17:01:46, joth wrote:
> net::URLRequestJob is refcounted, so it feels like you should just use that to
> keep the request alive while the async posttask is in flight, rather than add
> weak_ptr semantics into the class too. If there's a specific reason
ref-counting
> doesn't work, you should add a comment (here, or where the task is posted,
> whichever makes more sense).
> 
> (If the java-side looses interest and gets GCed I think the jweaks will
> automatically turn this into a no-op)

I was following the practice that the majority of other request job
implementations employ. I have figured that out from doing a code search for
"public net::URLRequestJob" WeakPtrFactory package:^chrome$

This just seems to be safer than forcing the class to hold a reference to itself
while the task is in flight.

[mkosiba (inactive), 2012-10-15 17:24:03]

https://codereview.chromium.org/11147021/diff/1/android_webview/native/androi...
File android_webview/native/android_stream_reader_url_request_job.h (right):

https://codereview.chromium.org/11147021/diff/1/android_webview/native/androi...
android_webview/native/android_stream_reader_url_request_job.h:78:
base::WeakPtrFactory<AndroidStreamReaderURLRequestJob> weak_factory_;
On 2012/10/15 17:01:46, joth wrote:
> net::URLRequestJob is refcounted, so it feels like you should just use that to
> keep the request alive while the async posttask is in flight, rather than add
> weak_ptr semantics into the class too. If there's a specific reason
ref-counting
> doesn't work, you should add a comment (here, or where the task is posted,
> whichever makes more sense).
> 
> (If the java-side looses interest and gets GCed I think the jweaks will
> automatically turn this into a no-op)

hmm.. all of the other URLRequestJob impls use a weak ptr. Maybe if you use a
weak_ptr you're saying that it's OK to delete the job even though the
async_start is in flight (which I guess is remotely possible) rather than
requiring that it's around until it completes (at which point the data isn't
going to be processed either way).

The main concern here is probably about how the URLRequest stuff will behave if
the URLRequestJob lives longer than the URLRequest that it's associated to.

[joth, 2012-10-15 18:10:34]

ah OK if this is a standard pattern in URLRequestJobs then lgtm




On 15 October 2012 10:24, <mkosiba@chromium.org> wrote:

>
> https://codereview.chromium.**org/11147021/diff/1/android_**
>
webview/native/android_stream_**reader_url_request_job.h<https://codereview.chromium.org/11147021/diff/1/android_webview/native/android_stream_reader_url_request_job.h>
> File android_webview/native/**android_stream_reader_url_**request_job.h
> (right):
>
> https://codereview.chromium.**org/11147021/diff/1/android_**
>
webview/native/android_stream_**reader_url_request_job.h#**newcode78<https://codereview.chromium.org/11147021/diff/1/android_webview/native/android_stream_reader_url_request_job.h#newcode78>
> android_webview/native/**android_stream_reader_url_**request_job.h:78:
> base::WeakPtrFactory<**AndroidStreamReaderURLRequestJ**ob> weak_factory_;
> On 2012/10/15 17:01:46, joth wrote:
>
>> net::URLRequestJob is refcounted, so it feels like you should just use
>>
> that to
>
>> keep the request alive while the async posttask is in flight, rather
>>
> than add
>
>> weak_ptr semantics into the class too. If there's a specific reason
>>
> ref-counting
>
>> doesn't work, you should add a comment (here, or where the task is
>>
> posted,
>
>> whichever makes more sense).
>>
>
>  (If the java-side looses interest and gets GCed I think the jweaks
>>
> will
>
>> automatically turn this into a no-op)
>>
>
> hmm.. all of the other URLRequestJob impls use a weak ptr. Maybe if you
> use a weak_ptr you're saying that it's OK to delete the job even though
> the async_start is in flight (which I guess is remotely possible) rather
> than requiring that it's around until it completes (at which point the
> data isn't going to be processed either way).
>
> The main concern here is probably about how the URLRequest stuff will
> behave if the URLRequestJob lives longer than the URLRequest that it's
> associated to.
>
>
https://codereview.chromium.**org/11147021/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2012-10-15 18:47:33]

Change committed as 161907