Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(603)

Issue 1467603002: URLRequestJob: change ReadRawData contract (Closed)

Created:
5 years, 1 month ago by xunjieli
Modified:
5 years, 1 month ago
Reviewers:
mmenke
CC:
chromium-reviews, qsr+mojo_chromium.org, tzik, yzshen+watch_chromium.org, kinuko+watch, ben+mojo_chromium.org, jsbell+serviceworker_chromium.org, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, viettrungluu+watch_chromium.org, jam, abarth-chromium, darin-cc_chromium.org, android-webview-reviews_chromium.org, blink-worker-reviews_chromium.org, nhiroki, oshima+watch_chromium.org, zork+watch_chromium.org, michaeln, serviceworker-reviews, Aaron Boodman, kinuko+serviceworker, Paweł Hajdan Jr., horo+watch_chromium.org, darin (slow to review), kinuko+fileapi, davemoore+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

URLRequestJob: change ReadRawData contract Previously, the interface for URLRequestJob::ReadRawData was as follows: bool ReadRawData(IOBuffer*, int, int*) Subclasses were expected to signal completion of the ReadRawData call by calling NotifyDone, SetStatus, or maybe one of the other Notify* functions on URLRequestJob, most of which do internal housekeeping and also drive the URLRequest's state machine. This made it difficult to reason about the URLRequestJob's state machine and needlessly complicated most of URLRequestJob. The new interface is as follows: int ReadRawData(IOBuffer*, int) Subclasses are required to either: a) Return ERR_IO_PENDING, and call ReadRawDataComplete when the read completes in any way, or b) Return a count of bytes read >= 0, indicating synchronous success, or c) Return another error code < 0, indicating synchronous failure. This substantially narrows the interface between URLRequestJob and its subclasses and moves the logic for the URLRequest state machine largely into URLRequestJob. Also, the signature of URLRequestJob::ReadFilteredData and some other internal URLRequestJob helpers changes to propagate detailed error codes instead of coercing all errors to FAILED. TBR=michaeln@chromium.org,mnaganov@chromium.org,skyostil@chromium.org,eugenebut@chromium.org,davidben@chromium.org,falken@chromium.org,mtomasz@chromium.org, sky@chromium.org,jianli@chromium.org,zork@chromium.org,rdsmith@chromium.org BUG=553300 Committed: https://crrev.com/26ede96bb45057dbb767ba41abde6ee29b1fb1de Cr-Commit-Position: refs/heads/master@{#361158}

Patch Set 1 : Revert of Revert #

Patch Set 2 : Fix #

Total comments: 3

Patch Set 3 : Remove CHECK(!done_reading_called_) in http_transaction_test_util.cc #

Patch Set 4 : add done_reading_called_ to MockNetworkTransaction #

Unified diffs Side-by-side diffs Delta from patch set Stats (+875 lines, -925 lines) Patch
M android_webview/browser/net/android_stream_reader_url_request_job.h View 3 chunks +3 lines, -1 line 0 comments Download
M android_webview/browser/net/android_stream_reader_url_request_job.cc View 8 chunks +28 lines, -38 lines 0 comments Download
M chrome/browser/chromeos/fileapi/external_file_url_request_job.h View 2 chunks +7 lines, -1 line 0 comments Download
M chrome/browser/chromeos/fileapi/external_file_url_request_job.cc View 6 chunks +31 lines, -33 lines 0 comments Download
M content/browser/android/url_request_content_job.h View 4 chunks +4 lines, -2 lines 0 comments Download
M content/browser/android/url_request_content_job.cc View 7 chunks +33 lines, -48 lines 0 comments Download
M content/browser/appcache/appcache_url_request_job.h View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/appcache/appcache_url_request_job.cc View 3 chunks +3 lines, -11 lines 0 comments Download
M content/browser/fileapi/file_writer_delegate_unittest.cc View 1 chunk +3 lines, -5 lines 0 comments Download
M content/browser/net/view_http_cache_job_factory.cc View 3 chunks +6 lines, -17 lines 0 comments Download
M content/browser/service_worker/service_worker_read_from_cache_job.h View 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/service_worker/service_worker_read_from_cache_job.cc View 5 chunks +38 lines, -33 lines 0 comments Download
M content/browser/service_worker/service_worker_url_request_job.h View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/service_worker/service_worker_url_request_job.cc View 6 chunks +29 lines, -47 lines 0 comments Download
M content/browser/service_worker/service_worker_write_to_cache_job.h View 4 chunks +19 lines, -13 lines 0 comments Download
M content/browser/service_worker/service_worker_write_to_cache_job.cc View 15 chunks +95 lines, -110 lines 0 comments Download
M content/browser/streams/stream_url_request_job.h View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/streams/stream_url_request_job.cc View 5 chunks +26 lines, -37 lines 0 comments Download
M content/browser/webui/url_data_manager_backend.cc View 5 chunks +13 lines, -24 lines 0 comments Download
M content/test/net/url_request_abort_on_end_job.h View 1 chunk +1 line, -1 line 0 comments Download
M content/test/net/url_request_abort_on_end_job.cc View 2 chunks +7 lines, -10 lines 0 comments Download
M ios/web/webui/url_data_manager_ios_backend.cc View 3 chunks +12 lines, -24 lines 0 comments Download
M mojo/services/network/url_loader_impl_apptest.cc View 1 chunk +6 lines, -10 lines 0 comments Download
M net/http/http_transaction_test_util.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M net/http/http_transaction_test_util.cc View 1 2 3 4 chunks +5 lines, -0 lines 0 comments Download
M net/test/url_request/url_request_failed_job.h View 2 chunks +2 lines, -1 line 0 comments Download
M net/test/url_request/url_request_failed_job.cc View 2 chunks +22 lines, -29 lines 0 comments Download
M net/test/url_request/url_request_mock_data_job.h View 1 chunk +1 line, -1 line 0 comments Download
M net/test/url_request/url_request_mock_data_job.cc View 1 chunk +6 lines, -9 lines 0 comments Download
M net/test/url_request/url_request_slow_download_job.h View 1 chunk +1 line, -1 line 0 comments Download
M net/test/url_request/url_request_slow_download_job.cc View 2 chunks +10 lines, -17 lines 0 comments Download
M net/url_request/url_request_file_dir_job.h View 4 chunks +4 lines, -2 lines 0 comments Download
M net/url_request/url_request_file_dir_job.cc View 3 chunks +27 lines, -28 lines 0 comments Download
M net/url_request/url_request_file_job.h View 3 chunks +5 lines, -1 line 0 comments Download
M net/url_request/url_request_file_job.cc View 10 chunks +24 lines, -37 lines 0 comments Download
M net/url_request/url_request_ftp_job.h View 1 chunk +1 line, -1 line 0 comments Download
M net/url_request/url_request_ftp_job.cc View 4 chunks +6 lines, -25 lines 0 comments Download
M net/url_request/url_request_http_job.h View 2 chunks +1 line, -4 lines 0 comments Download
M net/url_request/url_request_http_job.cc View 4 chunks +12 lines, -31 lines 0 comments Download
M net/url_request/url_request_job.h View 7 chunks +53 lines, -40 lines 0 comments Download
M net/url_request/url_request_job.cc View 1 16 chunks +132 lines, -99 lines 0 comments Download
M net/url_request/url_request_job_unittest.cc View 1 5 chunks +87 lines, -0 lines 0 comments Download
M net/url_request/url_request_simple_job.h View 2 chunks +1 line, -2 lines 0 comments Download
M net/url_request/url_request_simple_job.cc View 3 chunks +11 lines, -24 lines 0 comments Download
M net/url_request/url_request_status.h View 2 chunks +8 lines, -0 lines 0 comments Download
M net/url_request/url_request_status.cc View 1 chunk +15 lines, -0 lines 0 comments Download
M net/url_request/url_request_test_job.h View 1 chunk +1 line, -1 line 0 comments Download
M net/url_request/url_request_test_job.cc View 4 chunks +11 lines, -19 lines 0 comments Download
M storage/browser/blob/blob_url_request_job.h View 1 chunk +1 line, -1 line 0 comments Download
M storage/browser/blob/blob_url_request_job.cc View 3 chunks +14 lines, -28 lines 0 comments Download
M storage/browser/fileapi/file_system_dir_url_request_job.h View 1 chunk +1 line, -1 line 0 comments Download
M storage/browser/fileapi/file_system_dir_url_request_job.cc View 6 chunks +8 lines, -13 lines 0 comments Download
M storage/browser/fileapi/file_system_url_request_job.h View 4 chunks +3 lines, -2 lines 0 comments Download
M storage/browser/fileapi/file_system_url_request_job.cc View 10 chunks +31 lines, -39 lines 0 comments Download

Messages

Total messages: 20 (8 generated)
xunjieli
Matt, I am planning to TBR other owners since the rest of the files did ...
5 years, 1 month ago (2015-11-23 15:00:13 UTC) #3
xunjieli
https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transaction_test_util.cc File net/http/http_transaction_test_util.cc (right): https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transaction_test_util.cc#newcode214 net/http/http_transaction_test_util.cc:214: CHECK(!done_reading_called_); I can't add CHECK(!done_reading_called_) here, because done_reading_called is ...
5 years, 1 month ago (2015-11-23 15:37:56 UTC) #4
mmenke
https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transaction_test_util.cc File net/http/http_transaction_test_util.cc (right): https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transaction_test_util.cc#newcode214 net/http/http_transaction_test_util.cc:214: CHECK(!done_reading_called_); On 2015/11/23 15:37:56, xunjieli wrote: > I can't ...
5 years, 1 month ago (2015-11-23 16:06:02 UTC) #5
mmenke
On 2015/11/23 16:06:02, mmenke wrote: > https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transaction_test_util.cc > File net/http/http_transaction_test_util.cc (right): > > https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transaction_test_util.cc#newcode214 > ...
5 years, 1 month ago (2015-11-23 16:07:55 UTC) #6
xunjieli
SGTM. PTAL. https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transaction_test_util.cc File net/http/http_transaction_test_util.cc (right): https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transaction_test_util.cc#newcode214 net/http/http_transaction_test_util.cc:214: CHECK(!done_reading_called_); On 2015/11/23 16:06:02, mmenke wrote: > ...
5 years, 1 month ago (2015-11-23 16:15:58 UTC) #7
mmenke
On 2015/11/23 16:15:58, xunjieli wrote: > SGTM. PTAL. > > https://codereview.chromium.org/1467603002/diff/20001/net/http/http_transaction_test_util.cc > File net/http/http_transaction_test_util.cc (right): ...
5 years, 1 month ago (2015-11-23 16:43:04 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1467603002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1467603002/60001
5 years, 1 month ago (2015-11-23 16:46:35 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1467603002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1467603002/60001
5 years, 1 month ago (2015-11-23 16:48:04 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/132796)
5 years, 1 month ago (2015-11-23 18:34:21 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1467603002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1467603002/60001
5 years, 1 month ago (2015-11-23 18:52:54 UTC) #18
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 1 month ago (2015-11-23 19:39:21 UTC) #19
commit-bot: I haz the power
5 years, 1 month ago (2015-11-23 19:40:22 UTC) #20
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/26ede96bb45057dbb767ba41abde6ee29b1fb1de
Cr-Commit-Position: refs/heads/master@{#361158}

Powered by Google App Engine
This is Rietveld 408576698