Remove URLRequest::OrphanJob().

Remove URLRequest::OrphanJob().

This is a relic of a bygone era, when URLRequestJob was refcounted, and
could outlive its URLRequest. Now we can just delete the URLRequestJob.

Also remove a couple checks in URLRequestHttpJob and
ServiceWorkerURLRequestJob that their request is not NULL, as the checks
are no longer needed, for the same reason.

BUG=NONE

Committed: https://crrev.com/7ce675a75cf2f2e78a45e680c4cf22d8df521484
Cr-Commit-Position: refs/heads/master@{#430301}

[mmenke, 2016-11-03 19:42:51]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 19:44:03]

Dry run: 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-11-03 19:52:09]

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

[commit-bot: I haz the power, 2016-11-03 19:52:10]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mmenke, 2016-11-03 20:02:25]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 20:03:35]

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

[mmenke, 2016-11-03 20:31:06]

mmenke@chromium.org changed reviewers:
+ xunjieli@chromium.org

[mmenke, 2016-11-03 20:31:06]

Not exactly an important CL, just noticed I missed some stuff when removing
reference counting URLRequestJob a while back.

https://codereview.chromium.org/2480563002/diff/20001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/2480563002/diff/20001/net/url_request/url_req...
net/url_request/url_request.cc:190: context_->url_requests()->erase(this);
This admittedly isn't really related to the CL, just seems like we shouldn't be
CHECKing here, unless we have a reason to believe this isn't working, or it's
particularly important for some reason.

[commit-bot: I haz the power, 2016-11-03 20:47:28]

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

[commit-bot: I haz the power, 2016-11-03 20:47:28]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[mmenke, 2016-11-03 20:53:05]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 20:53:42]

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

[mmenke, 2016-11-03 21:01:18]

mmenke@chromium.org changed reviewers:
+ falken@chromium.org

[mmenke, 2016-11-03 21:01:18]

[+falken]:  PTAL.

https://codereview.chromium.org/2480563002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/2480563002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:75:
DCHECK_EQ(did_notify_started_, did_notify_finished_);
This is not a guarantee that URLRequest makes.

[mmenke, 2016-11-03 21:01:48]

Oops, falken:  PTAL at
	content/browser/service_worker/service_worker_write_to_cache_job.cc

[commit-bot: I haz the power, 2016-11-03 22:33:09]

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

[commit-bot: I haz the power, 2016-11-03 22:33:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2016-11-03 23:55:25]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 23:56:14]

Dry run: 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-11-04 02:38:21]

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

[commit-bot: I haz the power, 2016-11-04 02:38:21]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[falken, 2016-11-04 05:28:50]

lgtm. This also means we can remove the following code in
ServiceWorkerURLRequestJob::DidDispatchFetchEvent, right?

  // Check if we're not orphaned.
  if (!request()) {
    RecordResult(ServiceWorkerMetrics::REQUEST_JOB_ERROR_NO_REQUEST);
    return;
  }

https://codereview.chromium.org/2480563002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/2480563002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:75:
DCHECK_EQ(did_notify_started_, did_notify_finished_);
On 2016/11/03 21:01:18, mmenke wrote:
> This is not a guarantee that URLRequest makes.

Thanks for fixing this. Just to make sure I understand, now that we call Kill()
here the DCHECK is valid (since Kill() sets did_notify_finished_ if
did_notify_started is true), right?

[mmenke, 2016-11-04 14:58:17]

Description was changed from

==========
Remove URLRequest::OrphanJob().

This is a relic of a bygone era, when URLRequestJob was refcounted, and
could outlive its URLRequest. Now we can just delte the URLRequestJob.

Also remove a couple checks in URLRequestHttpJob that its request is
not NULL, as they're no longer needed for the same reason.

BUG=NONE
==========

to

==========
Remove URLRequest::OrphanJob().

This is a relic of a bygone era, when URLRequestJob was refcounted, and
could outlive its URLRequest. Now we can just delete the URLRequestJob.

Also remove a couple checks in URLRequestHttpJob that its request is
not NULL, as they're no longer needed for the same reason.

BUG=NONE
==========

[mmenke, 2016-11-04 15:03:10]

On 2016/11/04 05:28:50, falken wrote:
> lgtm. This also means we can remove the following code in
> ServiceWorkerURLRequestJob::DidDispatchFetchEvent, right?
> 
>   // Check if we're not orphaned.
>   if (!request()) {
>     RecordResult(ServiceWorkerMetrics::REQUEST_JOB_ERROR_NO_REQUEST);
>     return;
>   }

Correct.  I've gone ahead and removed it in this cl, along with another
request() is NULL check. I've left in the metric value, though.

https://codereview.chromium.org/2480563002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/2480563002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:75:
DCHECK_EQ(did_notify_started_, did_notify_finished_);
On 2016/11/04 05:28:49, falken wrote:
> On 2016/11/03 21:01:18, mmenke wrote:
> > This is not a guarantee that URLRequest makes.
> 
> Thanks for fixing this. Just to make sure I understand, now that we call
Kill()
> here the DCHECK is valid (since Kill() sets did_notify_finished_ if
> did_notify_started is true), right?

Correct.  The DCHECK was relying on Kill() always being called before the
request is destroyed, which URLRequest used to do (Needlessly - it was needed
before we released the URLRequest's reference when it was refcounted, but now
only this code requires it).  So calling Kill() here, at the start of the
destructor, means that this DCHECK is still correct.  I'm not sure whether it's
still needed or not, so I'm keeping the DCHECK around.

[mmenke, 2016-11-04 15:03:51]

Description was changed from

==========
Remove URLRequest::OrphanJob().

This is a relic of a bygone era, when URLRequestJob was refcounted, and
could outlive its URLRequest. Now we can just delete the URLRequestJob.

Also remove a couple checks in URLRequestHttpJob that its request is
not NULL, as they're no longer needed for the same reason.

BUG=NONE
==========

to

==========
Remove URLRequest::OrphanJob().

This is a relic of a bygone era, when URLRequestJob was refcounted, and
could outlive its URLRequest. Now we can just delete the URLRequestJob.

Also remove a couple checks in URLRequestHttpJob and
ServiceWorkerURLRequestJob that their request is not NULL, as the checks
are no longer needed, for the same reason.

BUG=NONE
==========

[mmenke, 2016-11-04 15:04:35]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 15:04:55]

Dry run: 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-11-04 16:27:19]

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

[commit-bot: I haz the power, 2016-11-04 16:27:19]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-11-07 15:09:08]

lgtm. 
Is URLRequestJob::Kill() something we can get rid of in the future? After
removal of OrphanJob(), Kill() is only used in cancellation by the request, and
it seems odd that URLRequestJob::Kill() notifies the request when it's being
canceled by the request.

[mmenke, 2016-11-07 15:12:46]

On 2016/11/07 15:09:08, xunjieli wrote:
> lgtm. 
> Is URLRequestJob::Kill() something we can get rid of in the future? After
> removal of OrphanJob(), Kill() is only used in cancellation by the request,
and
> it seems odd that URLRequestJob::Kill() notifies the request when it's being
> canceled by the request.

In addition to the notification, Kill() cancels all pending notifications. 
Unless we make deleting the job the only way to cancel it, we'll need to keep
Kill(), even if we get rid of that notification.

[mmenke, 2016-11-07 15:12:51]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-11-07 15:12:52]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2480563002/#ps60001
(title: "Response to comments")

[commit-bot: I haz the power, 2016-11-07 15:13:13]

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

[xunjieli, 2016-11-07 15:28:21]

On 2016/11/07 15:12:46, mmenke wrote:
> On 2016/11/07 15:09:08, xunjieli wrote:
> > lgtm. 
> > Is URLRequestJob::Kill() something we can get rid of in the future? After
> > removal of OrphanJob(), Kill() is only used in cancellation by the request,
> and
> > it seems odd that URLRequestJob::Kill() notifies the request when it's being
> > canceled by the request.
> 
> In addition to the notification, Kill() cancels all pending notifications. 
> Unless we make deleting the job the only way to cancel it, we'll need to keep
> Kill(), even if we get rid of that notification.

Ok. that makes sense. A minor improvement could be to rename Kill() to Cancel() 
(not in this CL) so it's more inlined with the rest of the stack.

[mmenke, 2016-11-07 15:30:48]

On 2016/11/07 15:28:21, xunjieli wrote:
> On 2016/11/07 15:12:46, mmenke wrote:
> > On 2016/11/07 15:09:08, xunjieli wrote:
> > > lgtm. 
> > > Is URLRequestJob::Kill() something we can get rid of in the future? After
> > > removal of OrphanJob(), Kill() is only used in cancellation by the
request,
> > and
> > > it seems odd that URLRequestJob::Kill() notifies the request when it's
being
> > > canceled by the request.
> > 
> > In addition to the notification, Kill() cancels all pending notifications. 
> > Unless we make deleting the job the only way to cancel it, we'll need to
keep
> > Kill(), even if we get rid of that notification.
> 
> Ok. that makes sense. A minor improvement could be to rename Kill() to
Cancel() 
> (not in this CL) so it's more inlined with the rest of the stack.

That sounds like a good idea to me.  We're doing more work in this space, and
once we have a better idea just where we're going to end up, I'll do the rename,
if we still need the method.

[commit-bot: I haz the power, 2016-11-07 17:08:59]

Description was changed from

==========
Remove URLRequest::OrphanJob().

This is a relic of a bygone era, when URLRequestJob was refcounted, and
could outlive its URLRequest. Now we can just delete the URLRequestJob.

Also remove a couple checks in URLRequestHttpJob and
ServiceWorkerURLRequestJob that their request is not NULL, as the checks
are no longer needed, for the same reason.

BUG=NONE
==========

to

==========
Remove URLRequest::OrphanJob().

This is a relic of a bygone era, when URLRequestJob was refcounted, and
could outlive its URLRequest. Now we can just delete the URLRequestJob.

Also remove a couple checks in URLRequestHttpJob and
ServiceWorkerURLRequestJob that their request is not NULL, as the checks
are no longer needed, for the same reason.

BUG=NONE
==========

[commit-bot: I haz the power, 2016-11-07 17:09:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-07 17:13:42]

Description was changed from

==========
Remove URLRequest::OrphanJob().

This is a relic of a bygone era, when URLRequestJob was refcounted, and
could outlive its URLRequest. Now we can just delete the URLRequestJob.

Also remove a couple checks in URLRequestHttpJob and
ServiceWorkerURLRequestJob that their request is not NULL, as the checks
are no longer needed, for the same reason.

BUG=NONE
==========

to

==========
Remove URLRequest::OrphanJob().

This is a relic of a bygone era, when URLRequestJob was refcounted, and
could outlive its URLRequest. Now we can just delete the URLRequestJob.

Also remove a couple checks in URLRequestHttpJob and
ServiceWorkerURLRequestJob that their request is not NULL, as the checks
are no longer needed, for the same reason.

BUG=NONE

Committed: https://crrev.com/7ce675a75cf2f2e78a45e680c4cf22d8df521484
Cr-Commit-Position: refs/heads/master@{#430301}
==========

[commit-bot: I haz the power, 2016-11-07 17:13:43]

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