Fix webkit URLRequestJob subtypes to handle Kill() correctly.

Fix webkit URLRequestJob subtypes to handle Kill() correctly.

Kill() should prevent calling back into the delegate.  So we cancel pending callbacks.

BUG=63692
TEST=existing

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

[willchan no longer on Chromium, 2010-12-06 21:23:18]

[+kinuko]

Not sure if Michael's on vacation or just otherwise slow.  Adding kinuko.

[willchan no longer on Chromium, 2010-12-06 21:23:56]

On 2010/12/06 21:23:18, willchan wrote:
> [+kinuko]
> 
> Not sure if Michael's on vacation or just otherwise slow.  Adding kinuko.

Er, I meant busy, not slow :P

[michaeln1, 2010-12-06 21:35:11]

Those don't seem mutually exclusive :)

On Mon, Dec 6, 2010 at 1:23 PM,  <willchan@chromium.org> wrote:
> On 2010/12/06 21:23:18, willchan wrote:
>>
>> [+kinuko]
>
>> Not sure if Michael's on vacation or just otherwise slow.  Adding kinuko.
>
> Er, I meant busy, not slow :P
>
> http://codereview.chromium.org/5545003/
>

[michaeln, 2010-12-06 22:51:38]

http://codereview.chromium.org/5545003/diff/1/webkit/appcache/appcache_url_re...
File webkit/appcache/appcache_url_request_job.cc (right):

http://codereview.chromium.org/5545003/diff/1/webkit/appcache/appcache_url_re...
webkit/appcache/appcache_url_request_job.cc:82: if (has_been_killed())
I think the changes to this job subclass aren't needed. The other classes look
good, but the mods to this class could be removed from the CL. Even with the
method_factory and the call to revokeAll(), we still need this test for
has_been_killed(), so the changes give us two different ways of doing the same
thing (halting progress after Kill). I'd vote to leave it as is with just one
way of not progressing after kill has been called. WDYT?

[willchan no longer on Chromium, 2010-12-07 00:52:38]

Thanks for the review!

http://codereview.chromium.org/5545003/diff/1/webkit/appcache/appcache_url_re...
File webkit/appcache/appcache_url_request_job.cc (right):

http://codereview.chromium.org/5545003/diff/1/webkit/appcache/appcache_url_re...
webkit/appcache/appcache_url_request_job.cc:82: if (has_been_killed())
On 2010/12/06 22:51:38, michaeln wrote:
> I think the changes to this job subclass aren't needed. The other classes look
> good, but the mods to this class could be removed from the CL. Even with the
> method_factory and the call to revokeAll(), we still need this test for
> has_been_killed(), so the changes give us two different ways of doing the same
> thing (halting progress after Kill). I'd vote to leave it as is with just one
> way of not progressing after kill has been called. WDYT?

Hm, I see.  That's unfortunate that we still need has_been_killed().  But I'd
still like to keep my change if you don't mind (do you think it'll confuse the
code much?).  The reason is because I'm trying to stop refcounting
URLRequestJob, and NewRunnableMethod() is calling AddRef() and Release() on
|this|.  The method_factory prevents that.

PS: In the future, I may try to refactor the code to eliminate the necessity for
has_been_killed(), since I want to simply delete the object instead of calling
Kill() on it.  I will need to read this object more carefully in the future
since it appears appcache code calls Kill() itself.

[michaeln, 2010-12-07 01:18:17]

On 2010/12/07 00:52:38, willchan wrote:
> Thanks for the review!
> 
>
http://codereview.chromium.org/5545003/diff/1/webkit/appcache/appcache_url_re...
> File webkit/appcache/appcache_url_request_job.cc (right):
> 
>
http://codereview.chromium.org/5545003/diff/1/webkit/appcache/appcache_url_re...
> webkit/appcache/appcache_url_request_job.cc:82: if (has_been_killed())
> On 2010/12/06 22:51:38, michaeln wrote:
> > I think the changes to this job subclass aren't needed. The other classes
look
> > good, but the mods to this class could be removed from the CL. Even with the
> > method_factory and the call to revokeAll(), we still need this test for
> > has_been_killed(), so the changes give us two different ways of doing the
same
> > thing (halting progress after Kill). I'd vote to leave it as is with just
one
> > way of not progressing after kill has been called. WDYT?
> 
> Hm, I see.  That's unfortunate that we still need has_been_killed().  But I'd
> still like to keep my change if you don't mind (do you think it'll confuse the
> code much?).  The reason is because I'm trying to stop refcounting
> URLRequestJob, and NewRunnableMethod() is calling AddRef() and Release() on
> |this|.  The method_factory prevents that.
> 
> PS: In the future, I may try to refactor the code to eliminate the necessity
for
> has_been_killed(), since I want to simply delete the object instead of calling
> Kill() on it.  I will need to read this object more carefully in the future
> since it appears appcache code calls Kill() itself.


lgtm

I see, I didn't understand your goal was to get rid of refcounting on that
object. I don't think your changes are too confusing at all, they just seemed
unnecessary. Fyi, the AppCacheRequestHandler produces this class of 'job' and
hangs on to a reference, so to get rid of refcounting that will have to be poked
at.

[willchan no longer on Chromium, 2010-12-07 01:23:57]

On Mon, Dec 6, 2010 at 5:18 PM, <michaeln@chromium.org> wrote:

> On 2010/12/07 00:52:38, willchan wrote:
>
>> Thanks for the review!
>>
>
>
>
>
http://codereview.chromium.org/5545003/diff/1/webkit/appcache/appcache_url_re...
>
>> File webkit/appcache/appcache_url_request_job.cc (right):
>>
>
>
>
>
http://codereview.chromium.org/5545003/diff/1/webkit/appcache/appcache_url_re...
>
>> webkit/appcache/appcache_url_request_job.cc:82: if (has_been_killed())
>> On 2010/12/06 22:51:38, michaeln wrote:
>> > I think the changes to this job subclass aren't needed. The other
>> classes
>>
> look
>
>> > good, but the mods to this class could be removed from the CL. Even with
>> the
>> > method_factory and the call to revokeAll(), we still need this test for
>> > has_been_killed(), so the changes give us two different ways of doing
>> the
>>
> same
>
>> > thing (halting progress after Kill). I'd vote to leave it as is with
>> just
>>
> one
>
>> > way of not progressing after kill has been called. WDYT?
>>
>
>  Hm, I see.  That's unfortunate that we still need has_been_killed().  But
>> I'd
>> still like to keep my change if you don't mind (do you think it'll confuse
>> the
>> code much?).  The reason is because I'm trying to stop refcounting
>> URLRequestJob, and NewRunnableMethod() is calling AddRef() and Release()
>> on
>> |this|.  The method_factory prevents that.
>>
>
>  PS: In the future, I may try to refactor the code to eliminate the
>> necessity
>>
> for
>
>> has_been_killed(), since I want to simply delete the object instead of
>> calling
>> Kill() on it.  I will need to read this object more carefully in the
>> future
>> since it appears appcache code calls Kill() itself.
>>
>
>
> lgtm
>
> I see, I didn't understand your goal was to get rid of refcounting on that
> object. I don't think your changes are too confusing at all, they just
> seemed
> unnecessary. Fyi, the AppCacheRequestHandler produces this class of 'job'
> and
> hangs on to a reference, so to get rid of refcounting that will have to be
> poked
> at.
>
>
Great, thanks!  I'll probably ask you about that a few months from now or
whenever I get around to it :)


>
>
>
> http://codereview.chromium.org/5545003/
>