Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2857733002:
iOS Cronet: Put cap on the buffer size passed to onReadCompleted delegate (Closed)
|
Created:
3 years, 7 months ago by kapishnikov Modified:
3 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, ios-reviews_chromium.org, net-reviews_chromium.org, droger Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptioniOS Cronet: Put cap on the buffer size passed to onReadCompleted delegate
The change also allows the client to start handling the received data sooner.
BUG=717551
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2857733002
Cr-Commit-Position: refs/heads/master@{#471319}
Committed: https://chromium.googlesource.com/chromium/src/+/922dd693b59ee99082ab9bab9daab3822d03047a
Patch Set 1 #Patch Set 2 : Improved Cronet unit test bug fix #
Total comments: 12
Patch Set 3 : Addressed comments #
Total comments: 4
Patch Set 4 : DCHECK #
Messages
Total messages: 25 (13 generated)
Description was changed from ========== iOS Cronet: Put cap on the buffer size passed to onReadCompleted delegate BUG=717551 ========== to ========== iOS Cronet: Put cap on the buffer size passed to onReadCompleted delegate BUG=717551 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Description was changed from ========== iOS Cronet: Put cap on the buffer size passed to onReadCompleted delegate BUG=717551 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== iOS Cronet: Put cap on the buffer size passed to onReadCompleted delegate The change also allows the client to start handling the received data sooner. BUG=717551 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
kapishnikov@chromium.org changed reviewers: + ellyjones@chromium.org, lilyhoughton@chromium.org, mef@chromium.org
https://codereview.chromium.org/2857733002/diff/20001/components/cronet/ios/t... File components/cronet/ios/test/cronet_http_test.mm (right): https://codereview.chromium.org/2857733002/diff/20001/components/cronet/ios/t... components/cronet/ios/test/cronet_http_test.mm:213: long size = 10 * 1024 * 1024; Where does this number come from? https://codereview.chromium.org/2857733002/diff/20001/ios/net/crn_http_protoc... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2857733002/diff/20001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:522: DCHECK_EQ(net_request_, request); Why does OnReadCompleted take request as an argument at all? https://codereview.chromium.org/2857733002/diff/20001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:559: if (bytes_read == 0) { Does it make sense for this to be |if (bytes_read == net::OK)|? https://codereview.chromium.org/2857733002/diff/20001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:565: if (bytes_read != net::OK && bytes_read != net::ERR_IO_PENDING) { I think comparing bytes_read to both 0 and net::OK makes the control flow a little confusing. And since (bytes_read == 0) and (bytes_read != net::OK) are mutually exclusive anyway, does it make more sense to have this be else if (bytes_read != net::ERR_IO_PENDING) ?
https://codereview.chromium.org/2857733002/diff/20001/ios/net/crn_http_protoc... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2857733002/diff/20001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:522: DCHECK_EQ(net_request_, request); On 2017/05/05 16:33:05, lilyhoughton wrote: > Why does OnReadCompleted take request as an argument at all? It is implementing UrlRequest::Delegate, which allows single delegate to handle multiple requests. https://codereview.chromium.org/2857733002/diff/20001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:528: uint64_t total_byte_read = 0; nit: rename to total_bytes_read for consistency. https://codereview.chromium.org/2857733002/diff/20001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:538: data_length = [data length]; move NSUInteger data_length declaration here to limit scope or just inline it below?
The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2857733002/diff/20001/components/cronet/ios/t... File components/cronet/ios/test/cronet_http_test.mm (right): https://codereview.chromium.org/2857733002/diff/20001/components/cronet/ios/t... components/cronet/ios/test/cronet_http_test.mm:213: long size = 10 * 1024 * 1024; On 2017/05/05 16:33:05, lilyhoughton wrote: > Where does this number come from? This is an arbitrary size of the response that we use for testing. The higher number would make the test run longer and consume more RAM. The lower number would give us less precision when we try to measure the elapsed time. https://codereview.chromium.org/2857733002/diff/20001/ios/net/crn_http_protoc... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2857733002/diff/20001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:528: uint64_t total_byte_read = 0; On 2017/05/05 17:08:19, mef wrote: > nit: rename to total_bytes_read for consistency. Done. https://codereview.chromium.org/2857733002/diff/20001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:538: data_length = [data length]; On 2017/05/05 17:08:19, mef wrote: > move NSUInteger data_length declaration here to limit scope or just inline it > below? Done. https://codereview.chromium.org/2857733002/diff/20001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:559: if (bytes_read == 0) { On 2017/05/05 16:33:05, lilyhoughton wrote: > Does it make sense for this to be |if (bytes_read == net::OK)|? Changed it. https://codereview.chromium.org/2857733002/diff/20001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:565: if (bytes_read != net::OK && bytes_read != net::ERR_IO_PENDING) { On 2017/05/05 16:33:05, lilyhoughton wrote: > I think comparing bytes_read to both 0 and net::OK makes the control flow a > little confusing. > > And since (bytes_read == 0) and (bytes_read != net::OK) are mutually exclusive > anyway, does it make more sense to have this be > else if (bytes_read != net::ERR_IO_PENDING) > ? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
kapishnikov@chromium.org changed reviewers: + droger@chromium.org
Adding David to take a look as the iOS/net owner.
sdefresne@chromium.org changed reviewers: + marq@chromium.org - droger@chromium.org
-droger (→ cc), +marq as OWNERS of ios/net as droger is OOO till 29th of May.
https://codereview.chromium.org/2857733002/diff/40001/ios/net/crn_http_protoc... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2857733002/diff/40001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:54: // be less or equal to |kClientMaxBufferSize|. Why not express kIOBufferSize in terms of the max buffer size to ensure this? (or add a DCHECK somewhere) https://codereview.chromium.org/2857733002/diff/40001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:533: // |kIOBufferSize|) is always smaller or equal to |kClientMaxBufferSize|. It seems to me that if this condition isn't true (if bytes_read is > max buffer size), then the outer while() will never exit. Maybe it's worth a DCHECK safety valve to catch this?
https://codereview.chromium.org/2857733002/diff/40001/ios/net/crn_http_protoc... File ios/net/crn_http_protocol_handler.mm (right): https://codereview.chromium.org/2857733002/diff/40001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:54: // be less or equal to |kClientMaxBufferSize|. On 2017/05/11 11:20:34, marq (ping after 24h) wrote: > Why not express kIOBufferSize in terms of the max buffer size to ensure this? > > (or add a DCHECK somewhere) Great suggestion. I expressed kClientMaxBufferSize in terms of kIOBufferSize. It will also make it multiple of kIOBufferSize, which can be beneficial for the performance. https://codereview.chromium.org/2857733002/diff/40001/ios/net/crn_http_protoc... ios/net/crn_http_protocol_handler.mm:533: // |kIOBufferSize|) is always smaller or equal to |kClientMaxBufferSize|. On 2017/05/11 11:20:34, marq (ping after 24h) wrote: > It seems to me that if this condition isn't true (if bytes_read is > max buffer > size), then the outer while() will never exit. Maybe it's worth a DCHECK safety > valve to catch this? Done.
lgtm
The CQ bit was checked by kapishnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org Link to the patchset: https://codereview.chromium.org/2857733002/#ps60001 (title: "DCHECK")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494602801804010,
"parent_rev": "e65724e4537aa0e42d14ba0a7fe19d077158f9ea", "commit_rev":
"922dd693b59ee99082ab9bab9daab3822d03047a"}
Message was sent while issue was closed.
Description was changed from ========== iOS Cronet: Put cap on the buffer size passed to onReadCompleted delegate The change also allows the client to start handling the received data sooner. BUG=717551 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== iOS Cronet: Put cap on the buffer size passed to onReadCompleted delegate The change also allows the client to start handling the received data sooner. BUG=717551 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2857733002 Cr-Commit-Position: refs/heads/master@{#471319} Committed: https://chromium.googlesource.com/chromium/src/+/922dd693b59ee99082ab9bab9daa... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/922dd693b59ee99082ab9bab9daa... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
