New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 688481 link

Starred by 2 users
Status: Available
Owner: ----
Cc:
█ juliatut...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Remove URLRequestJob::GetResponseCode

Project Member Reported by mmenke@chromium.org, Feb 3 2017 
URLRequestJob::GetResponseCode returns the response code for a request.  Intuitively, it should be the same as URLRequest::response_info()->headers->GetResponseCode()...However, this is not the case.  For URLRequestHttpJob, it's set before response_info(), and this behavior is externally observable - it's set to the response code before the URLRequest's ResponseInfo is set, and the difference can be noticed during the NetworkDelegate::OnHeadersReceived call.  It also crashes when there is a ResponseInfo but no headers (Instead of, say, returning -1, which it does for other URLRequestJob types without headers).

This is weird and unexpected.  We should work towards being able to get rid of the method, and making URLRequest::GetResponseCode just grab it from URLRequest's ResponseInfo call.

Comment 1 by mmenke@chromium.org, Feb 3 2017

Labels: Fixit-Net
My suggested approach is to give URLRequestJob::GetResponseCode a reasonable default behavior, and then remove the implementations of this from subclasses a few at a time - I suspect URLRequestHttpJob will be the only difficult one, but others may require some work, too.
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 3 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a

commit 3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a
Author: Matt Menke <mmenke@chromium.org>
Date: Fri Feb 03 22:18:41 2017

Give URLRequestJob::GetResponseCode a better default implementation.

Also remove a couple overrides of the method that are no longer needed,
mostly from net test classes.  Others can be removed in followup CLs.

BUG=688481
R=juliatuttle@chromium.org

Review-Url: https://codereview.chromium.org/2676033002 .
Cr-Commit-Position: refs/heads/master@{#448087}

[modify] https://crrev.com/3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a/net/test/url_request/url_request_failed_job.cc
[modify] https://crrev.com/3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a/net/test/url_request/url_request_failed_job.h
[modify] https://crrev.com/3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a/net/test/url_request/url_request_hanging_read_job.cc
[modify] https://crrev.com/3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a/net/test/url_request/url_request_hanging_read_job.h
[modify] https://crrev.com/3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a/net/test/url_request/url_request_mock_data_job.cc
[modify] https://crrev.com/3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a/net/test/url_request/url_request_mock_data_job.h
[modify] https://crrev.com/3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a/net/test/url_request/url_request_mock_http_job.cc
[modify] https://crrev.com/3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a/net/test/url_request/url_request_mock_http_job.h
[modify] https://crrev.com/3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a/net/url_request/test_url_request_interceptor.cc
[modify] https://crrev.com/3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a/net/url_request/url_request_job.cc
[modify] https://crrev.com/3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a/net/url_request/url_request_redirect_job.cc
[modify] https://crrev.com/3e0a3877d7a54cb14e1646bacb923d2aab3c6b0a/net/url_request/url_request_redirect_job.h
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 3 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4f1a3523846b934a765fd169e6652a28088449be

commit 4f1a3523846b934a765fd169e6652a28088449be
Author: mmenke <mmenke@chromium.org>
Date: Fri Feb 03 22:53:18 2017

Remove BlobURLRequestJob::GetResponseCode().

The default implementation of URLRequestJob::GetResponseCode() is being
modified to (mostly) match what it currently does.

BUG=688481

Review-Url: https://codereview.chromium.org/2670413002
Cr-Commit-Position: refs/heads/master@{#448103}

[modify] https://crrev.com/4f1a3523846b934a765fd169e6652a28088449be/storage/browser/blob/blob_url_request_job.cc
[modify] https://crrev.com/4f1a3523846b934a765fd169e6652a28088449be/storage/browser/blob/blob_url_request_job.h
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 4 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9f3d62526e92be1ec627cdd4de25104a8e280a5e

commit 9f3d62526e92be1ec627cdd4de25104a8e280a5e
Author: mmenke <mmenke@chromium.org>
Date: Sat Feb 04 01:09:29 2017

ServiceWorker:  Remove URLRequestJob::GetResponseCode implementations.

The default implementation is being modified to get the response code
from the URLRequest's HttpResponseInfo, to ensure it's consistent, and
hopefully we'll eventually be able to remove the method from
URLRequestJob entirely.

BUG=688481

Review-Url: https://codereview.chromium.org/2670243003
Cr-Commit-Position: refs/heads/master@{#448138}

[modify] https://crrev.com/9f3d62526e92be1ec627cdd4de25104a8e280a5e/content/browser/service_worker/service_worker_read_from_cache_job.cc
[modify] https://crrev.com/9f3d62526e92be1ec627cdd4de25104a8e280a5e/content/browser/service_worker/service_worker_read_from_cache_job.h
[modify] https://crrev.com/9f3d62526e92be1ec627cdd4de25104a8e280a5e/content/browser/service_worker/service_worker_url_request_job.cc
[modify] https://crrev.com/9f3d62526e92be1ec627cdd4de25104a8e280a5e/content/browser/service_worker/service_worker_url_request_job.h
[modify] https://crrev.com/9f3d62526e92be1ec627cdd4de25104a8e280a5e/content/browser/service_worker/service_worker_write_to_cache_job.cc
[modify] https://crrev.com/9f3d62526e92be1ec627cdd4de25104a8e280a5e/content/browser/service_worker/service_worker_write_to_cache_job.h
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 17 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/859329ee3ecac2404c5d9358e4eda08d7d27fc03

commit 859329ee3ecac2404c5d9358e4eda08d7d27fc03
Author: mmenke <mmenke@chromium.org>
Date: Mon Apr 17 20:41:34 2017

Remove URLRequestJob::GetResponseCode implementations from AppCache.

URLRequestJob itself now has a default implementation, which gets
the response code from the headers returned by GetResponseInfo.
We eventually want to remove this method, since it's redundant,
and has weird behavior for just when it is and is not safe to
call.

BUG=688481

Review-Url: https://codereview.chromium.org/2824583003
Cr-Commit-Position: refs/heads/master@{#465011}

[modify] https://crrev.com/859329ee3ecac2404c5d9358e4eda08d7d27fc03/content/browser/appcache/appcache_request_handler_unittest.cc
[modify] https://crrev.com/859329ee3ecac2404c5d9358e4eda08d7d27fc03/content/browser/appcache/appcache_update_job_unittest.cc
[modify] https://crrev.com/859329ee3ecac2404c5d9358e4eda08d7d27fc03/content/browser/appcache/appcache_url_request_job.cc
[modify] https://crrev.com/859329ee3ecac2404c5d9358e4eda08d7d27fc03/content/browser/appcache/appcache_url_request_job.h
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 18 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e0f6ab79ba3d77e09f20175897bf0664b726fec0

commit e0f6ab79ba3d77e09f20175897bf0664b726fec0
Author: mmenke <mmenke@chromium.org>
Date: Tue Apr 18 18:12:12 2017

Remove URLRequestJob::GetResponseCode implementations.

URLRequestJob itself now has a default implementation, which gets
the response code from the headers returned by GetResponseInfo.
We eventually want to remove this method, since it's redundant,
and has weird behavior for just when it is and is not safe to
call.

BUG=688481

Review-Url: https://codereview.chromium.org/2824813002
Cr-Commit-Position: refs/heads/master@{#465292}

[modify] https://crrev.com/e0f6ab79ba3d77e09f20175897bf0664b726fec0/android_webview/browser/net/android_stream_reader_url_request_job.cc
[modify] https://crrev.com/e0f6ab79ba3d77e09f20175897bf0664b726fec0/android_webview/browser/net/android_stream_reader_url_request_job.h
[modify] https://crrev.com/e0f6ab79ba3d77e09f20175897bf0664b726fec0/components/domain_reliability/uploader_unittest.cc
[modify] https://crrev.com/e0f6ab79ba3d77e09f20175897bf0664b726fec0/headless/public/util/deterministic_http_protocol_handler.cc
[modify] https://crrev.com/e0f6ab79ba3d77e09f20175897bf0664b726fec0/headless/public/util/generic_url_request_job.cc
[modify] https://crrev.com/e0f6ab79ba3d77e09f20175897bf0664b726fec0/headless/public/util/generic_url_request_job.h
[modify] https://crrev.com/e0f6ab79ba3d77e09f20175897bf0664b726fec0/headless/public/util/generic_url_request_job_test.cc
[modify] https://crrev.com/e0f6ab79ba3d77e09f20175897bf0664b726fec0/headless/public/util/http_url_fetcher.cc
[modify] https://crrev.com/e0f6ab79ba3d77e09f20175897bf0664b726fec0/headless/public/util/testing/generic_url_request_mocks.cc
[modify] https://crrev.com/e0f6ab79ba3d77e09f20175897bf0664b726fec0/headless/public/util/testing/generic_url_request_mocks.h
[modify] https://crrev.com/e0f6ab79ba3d77e09f20175897bf0664b726fec0/headless/public/util/url_fetcher.cc
[modify] https://crrev.com/e0f6ab79ba3d77e09f20175897bf0664b726fec0/headless/public/util/url_fetcher.h
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 19 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db0751b8db4b09ffba0d7907dcb93311b2c596ec

commit db0751b8db4b09ffba0d7907dcb93311b2c596ec
Author: mmenke <mmenke@chromium.org>
Date: Wed Apr 19 14:07:34 2017

Remove URLRequestJob::GetResponseCode implementations from chrome/

URLRequestJob itself now has a default implementation, which gets
the response code from the headers returned by GetResponseInfo.
We eventually want to remove this method, since it's redundant,
and has weird behavior for just when it is and is not safe to
call.

BUG=688481

Review-Url: https://codereview.chromium.org/2821093002
Cr-Commit-Position: refs/heads/master@{#465587}

[modify] https://crrev.com/db0751b8db4b09ffba0d7907dcb93311b2c596ec/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc
[modify] https://crrev.com/db0751b8db4b09ffba0d7907dcb93311b2c596ec/chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc
[modify] https://crrev.com/db0751b8db4b09ffba0d7907dcb93311b2c596ec/chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h
[modify] https://crrev.com/db0751b8db4b09ffba0d7907dcb93311b2c596ec/chrome/browser/ssl/ssl_browser_tests.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 19 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f13168e3f095f995d58ac9baf810a71531fd10bf

commit f13168e3f095f995d58ac9baf810a71531fd10bf
Author: mmenke <mmenke@chromium.org>
Date: Wed Apr 19 14:13:59 2017

Remove URLRequestJob::GetResponseCode implementations
from streams and fileapi code.

URLRequestJob itself now has a default implementation, which gets
the response code from the headers returned by GetResponseInfo.
We eventually want to remove this method, since it's redundant,
and has weird behavior for just when it is and is not safe to
call.

BUG=688481

Review-Url: https://codereview.chromium.org/2822593003
Cr-Commit-Position: refs/heads/master@{#465588}

[modify] https://crrev.com/f13168e3f095f995d58ac9baf810a71531fd10bf/content/browser/streams/stream_url_request_job.cc
[modify] https://crrev.com/f13168e3f095f995d58ac9baf810a71531fd10bf/content/browser/streams/stream_url_request_job.h
[modify] https://crrev.com/f13168e3f095f995d58ac9baf810a71531fd10bf/storage/browser/fileapi/file_system_url_request_job.cc
[modify] https://crrev.com/f13168e3f095f995d58ac9baf810a71531fd10bf/storage/browser/fileapi/file_system_url_request_job.h
[modify] https://crrev.com/f13168e3f095f995d58ac9baf810a71531fd10bf/storage/browser/fileapi/file_writer_delegate_unittest.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 24 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ec95f5b430aac7baff41e4b34cda43116a438905

commit ec95f5b430aac7baff41e4b34cda43116a438905
Author: mmenke <mmenke@chromium.org>
Date: Mon Apr 24 15:33:33 2017

Remove URLRequestJob::GetResponseCode implementations outside of net/.

URLRequestJob itself now has a default implementation, which gets
the response code from the headers returned by GetResponseInfo.
We eventually want to remove this method, since it's redundant,
and has weird behavior for just when it is and is not safe to
call.

BUG=688481

Review-Url: https://codereview.chromium.org/2827233005
Cr-Commit-Position: refs/heads/master@{#466631}

[modify] https://crrev.com/ec95f5b430aac7baff41e4b34cda43116a438905/components/update_client/url_request_post_interceptor.cc
[modify] https://crrev.com/ec95f5b430aac7baff41e4b34cda43116a438905/extensions/browser/extension_protocols.cc
[modify] https://crrev.com/ec95f5b430aac7baff41e4b34cda43116a438905/ios/net/protocol_handler_util_unittest.mm
[modify] https://crrev.com/ec95f5b430aac7baff41e4b34cda43116a438905/ios/web/webui/url_data_manager_ios_backend.mm
[modify] https://crrev.com/ec95f5b430aac7baff41e4b34cda43116a438905/storage/browser/fileapi/file_writer_delegate_unittest.cc
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 28 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e47392b3d5fa37b785ea7494bc7bd32163ba9188

commit e47392b3d5fa37b785ea7494bc7bd32163ba9188
Author: mmenke <mmenke@chromium.org>
Date: Fri Apr 28 02:12:11 2017

Remove URLRequestJob::GetResponseCode implementations
from content, chrome, and net/.

URLRequestJob itself now has a default implementation, which gets
the response code from the headers returned by GetResponseInfo.
We eventually want to remove this method, since it's redundant,
and has weird behavior for just when it is and is not safe to
call.

BUG=688481

Review-Url: https://codereview.chromium.org/2835533002
Cr-Commit-Position: refs/heads/master@{#467839}

[modify] https://crrev.com/e47392b3d5fa37b785ea7494bc7bd32163ba9188/chrome/browser/android/offline_pages/offline_page_request_job.cc
[modify] https://crrev.com/e47392b3d5fa37b785ea7494bc7bd32163ba9188/chrome/browser/android/offline_pages/offline_page_request_job.h
[modify] https://crrev.com/e47392b3d5fa37b785ea7494bc7bd32163ba9188/content/browser/webui/url_data_manager_backend.cc
[modify] https://crrev.com/e47392b3d5fa37b785ea7494bc7bd32163ba9188/content/public/test/test_download_request_handler.cc
[modify] https://crrev.com/e47392b3d5fa37b785ea7494bc7bd32163ba9188/net/url_request/url_request_test_job.cc
[modify] https://crrev.com/e47392b3d5fa37b785ea7494bc7bd32163ba9188/net/url_request/url_request_test_job.h

Comment 11 by rdsmith@chromium.org, Feb 16 2018

Cc: -rdsmith@chromium.org

Comment 12 by mmenke@chromium.org, May 2 2018

Labels: Network-Triaged
►

Sign in to add a comment