Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2167853003:
[DevTools] Always report encodedDataLength in Network.ResponseReceived. (Closed)
|
Created:
4 years, 5 months ago by allada Modified:
3 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, pfeldman+blink_chromium.org, jam, Randy Smith (Not in Mondays), lushnikov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, darin-cc_chromium.org, pfeldman, devtools-reviews_chromium.org, loading-reviews_chromium.org, apavlov+blink_chromium.org, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Always report encodedDataLength in Network.ResponseReceived.
Currently, we only send encodedDataLength for redirects. However, we
already know how much we read when sending ResponseReceived. This will
improve reporting for failed requests, as they don't get loadingFinished
with full encodedDataLength.
Previous CL: https://codereview.chromium.org/2101073002
BUG=622018
Committed: https://crrev.com/71b2efb2261233d22779f8ee192b81e7a942fe2b
Cr-Commit-Position: refs/heads/master@{#417506}
Patch Set 1 : Fixed Header sizes in devtools #
Total comments: 10
Patch Set 2 : Fixed Header sizes in devtools #Patch Set 3 : Fixed Header sizes in devtools #Patch Set 4 : Merge remote-tracking branch 'origin/master' into HEADER_SIZE_FINAL #Patch Set 5 : Quic Tests #
Total comments: 29
Patch Set 6 : Changes #
Total comments: 23
Patch Set 7 : Changes #Patch Set 8 : Merge branch 'master' into HEADER_SIZE_FINAL #Patch Set 9 : changes #
Total comments: 8
Patch Set 10 : changes #
Total comments: 1
Messages
Total messages: 79 (44 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by allada@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...
allada@chromium.org changed reviewers: + dgozman@chromium.org, dpranke@chromium.org, mmenke@chromium.org
mmenke@chromium.org: Please review changes in /content/ and /net/ dgozman@chromium.org: Please review changes in /inspector/ dpranke@chromium.org: Please review changes in apache /conf/
https://codereview.chromium.org/2167853003/diff/80001/content/browser/loader/... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2167853003/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler.cc:358: response->head.encoded_data_length = request()->raw_header_size(); Hrm...I guess you can't use request()->GetTotalReceivedBytes() here because of mime sniffing? https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... File net/url_request/url_request.h (right): https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... net/url_request/url_request.h:651: // Gets the raw header size of the request. It's unclear what "raw header size" means. You should document this and rename it (over_the_wire_response_header_size()?) This is over-the-wire response (Not request) header size (0 if response was cached, or unable to parse headers). Some random value for HTTP/0.9 (Good thing we're removing support). It may or may not include protocol overhead (For H2 it does, for QUIC it does not, it does not include HTTPS overhead). It also may include a random number of bytes that we read after the end of an HTTP/1.x header, which may in fact exceed the size of the entire HTTP response. Hrm...This is starting to sound pretty iffy, as a web-exposed API...
https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... File net/url_request/url_request.h (right): https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... net/url_request/url_request.h:651: // Gets the raw header size of the request. On 2016/07/21 18:10:50, mmenke wrote: > It's unclear what "raw header size" means. You should document this and rename > it (over_the_wire_response_header_size()?) > > This is over-the-wire response (Not request) header size (0 if response was > cached, or unable to parse headers). Some random value for HTTP/0.9 (Good thing > we're removing support). It may or may not include protocol overhead (For H2 it > does, for QUIC it does not, it does not include HTTPS overhead). It also may > include a random number of bytes that we read after the end of an HTTP/1.x > header, which may in fact exceed the size of the entire HTTP response. > > Hrm...This is starting to sound pretty iffy, as a web-exposed API... Actually, I think with H2/QUIC, it may be able to include part of the body as well, at least with pushed streams. Not sure about other streams.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2167853003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right): https://codereview.chromium.org/2167853003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:632: encodedDataLength -= resourceData->rawHeaderSize(); I don't like this. Let's fix it async_handler instead. https://codereview.chromium.org/2167853003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/NetworkResourcesData.h (right): https://codereview.chromium.org/2167853003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/NetworkResourcesData.h:132: size_t dataLength() const; I believe this may depend on cached vs non-cached.
PTL https://codereview.chromium.org/2167853003/diff/80001/content/browser/loader/... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2167853003/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler.cc:358: response->head.encoded_data_length = request()->raw_header_size(); On 2016/07/21 18:10:50, mmenke wrote: > Hrm...I guess you can't use request()->GetTotalReceivedBytes() here because of > mime sniffing? I tried that originally but it was returning the number of bytes at that time, so if a server sent the headers and some content at the same time it would give back the size of the headers + size of content received. By moving it into a variable that is explicitly set when headers are parsed it is much more reliable. https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... File net/url_request/url_request.h (right): https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... net/url_request/url_request.h:651: // Gets the raw header size of the request. On 2016/07/21 18:47:02, mmenke wrote: > On 2016/07/21 18:10:50, mmenke wrote: > > It's unclear what "raw header size" means. You should document this and > rename > > it (over_the_wire_response_header_size()?) > > > > This is over-the-wire response (Not request) header size (0 if response was > > cached, or unable to parse headers). Some random value for HTTP/0.9 (Good > thing > > we're removing support). It may or may not include protocol overhead (For H2 > it > > does, for QUIC it does not, it does not include HTTPS overhead). It also may > > include a random number of bytes that we read after the end of an HTTP/1.x > > header, which may in fact exceed the size of the entire HTTP response. > > > > Hrm...This is starting to sound pretty iffy, as a web-exposed API... This property contains the value of the size in bytes of the header not including SSL overhead but does include H2/Quic/HTTP overhead. I am ok changing it, but I don't know a much better name since it's not actually "over the wire size". > > Actually, I think with H2/QUIC, it may be able to include part of the body as > well, at least with pushed streams. Not sure about other streams. From what I can see in H2 we are using the value that comes from SpdyStream which represents a resource. Unless content can be sent and put into this class before headers are processed it should be safe. With Quic it appears to do the same (QuicSpdyStream). I ran some tests on a simple Go H2 server I wrote and it appears to work as expected (tested multiple concurrent resources over same socket), but have not tested on complex H2 requests. https://codereview.chromium.org/2167853003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right): https://codereview.chromium.org/2167853003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:632: encodedDataLength -= resourceData->rawHeaderSize(); On 2016/07/21 21:07:59, dgozman wrote: > I don't like this. Let's fix it async_handler instead. Done. https://codereview.chromium.org/2167853003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/NetworkResourcesData.h (right): https://codereview.chromium.org/2167853003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/NetworkResourcesData.h:132: size_t dataLength() const; On 2016/07/21 21:07:59, dgozman wrote: > I believe this may depend on cached vs non-cached. I tested the code against cached connections and have a test for it and appears to work properly.
https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... File net/url_request/url_request.h (right): https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... net/url_request/url_request.h:651: // Gets the raw header size of the request. On 2016/07/22 17:19:32, Blaise wrote: > On 2016/07/21 18:47:02, mmenke wrote: > > On 2016/07/21 18:10:50, mmenke wrote: > > > It's unclear what "raw header size" means. You should document this and > > rename > > > it (over_the_wire_response_header_size()?) > > > > > > This is over-the-wire response (Not request) header size (0 if response was > > > cached, or unable to parse headers). Some random value for HTTP/0.9 (Good > > thing > > > we're removing support). It may or may not include protocol overhead (For > H2 > > it > > > does, for QUIC it does not, it does not include HTTPS overhead). It also > may > > > include a random number of bytes that we read after the end of an HTTP/1.x > > > header, which may in fact exceed the size of the entire HTTP response. > > > > > > Hrm...This is starting to sound pretty iffy, as a web-exposed API... > This property contains the value of the size in bytes of the header not > including SSL overhead but does include H2/Quic/HTTP overhead. I am ok changing > it, but I don't know a much better name since it's not actually "over the wire > size". > > > > > Actually, I think with H2/QUIC, it may be able to include part of the body as > > well, at least with pushed streams. Not sure about other streams. > > From what I can see in H2 we are using the value that comes from SpdyStream > which represents a resource. Unless content can be sent and put into this class > before headers are processed it should be safe. With Quic it appears to do the > same (QuicSpdyStream). I ran some tests on a simple Go H2 server I wrote and it > appears to work as expected (tested multiple concurrent resources over same > socket), but have not tested on complex H2 requests. Hrm...Digging into HttpStreamParser, it looks like this may actually work, since it doesn't actually return bytes read there. That having been said, I can't sign off on this without unit tests that this actually returns header size, and not over-the-wire bytes that we've read from the socket (For HTTP, in particular, we don't know header size, so we potentially have to read a lot more data before we learn the size of the headers), and updated API descriptions supporting this use case. This is something that seems like it could invisibly break, causing real regressions, but not failures, and no developers would notice it. If it's worth doing, it's worth trying to ensure it doesn't break. Could even make an explicit API to get header size, down to the HttpStream layer, to even further reduce risk of regression.
On 2016/07/22 18:50:09, mmenke wrote: > https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... > File net/url_request/url_request.h (right): > > https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... > net/url_request/url_request.h:651: // Gets the raw header size of the request. > On 2016/07/22 17:19:32, Blaise wrote: > > On 2016/07/21 18:47:02, mmenke wrote: > > > On 2016/07/21 18:10:50, mmenke wrote: > > > > It's unclear what "raw header size" means. You should document this and > > > rename > > > > it (over_the_wire_response_header_size()?) > > > > > > > > This is over-the-wire response (Not request) header size (0 if response > was > > > > cached, or unable to parse headers). Some random value for HTTP/0.9 (Good > > > thing > > > > we're removing support). It may or may not include protocol overhead (For > > H2 > > > it > > > > does, for QUIC it does not, it does not include HTTPS overhead). It also > > may > > > > include a random number of bytes that we read after the end of an HTTP/1.x > > > > header, which may in fact exceed the size of the entire HTTP response. > > > > > > > > Hrm...This is starting to sound pretty iffy, as a web-exposed API... > > This property contains the value of the size in bytes of the header not > > including SSL overhead but does include H2/Quic/HTTP overhead. I am ok > changing > > it, but I don't know a much better name since it's not actually "over the wire > > size". > > > > > > > > Actually, I think with H2/QUIC, it may be able to include part of the body > as > > > well, at least with pushed streams. Not sure about other streams. > > > > From what I can see in H2 we are using the value that comes from SpdyStream > > which represents a resource. Unless content can be sent and put into this > class > > before headers are processed it should be safe. With Quic it appears to do the > > same (QuicSpdyStream). I ran some tests on a simple Go H2 server I wrote and > it > > appears to work as expected (tested multiple concurrent resources over same > > socket), but have not tested on complex H2 requests. > > Hrm...Digging into HttpStreamParser, it looks like this may actually work, since > it doesn't actually return bytes read there. > > That having been said, I can't sign off on this without unit tests that this > actually returns header size, and not over-the-wire bytes that we've read from > the socket (For HTTP, in particular, we don't know header size, so we > potentially have to read a lot more data before we learn the size of the > headers), and updated API descriptions supporting this use case. > > This is something that seems like it could invisibly break, causing real > regressions, but not failures, and no developers would notice it. If it's worth > doing, it's worth trying to ensure it doesn't break. > > Could even make an explicit API to get header size, down to the HttpStream > layer, to even further reduce risk of regression. Test cases I'd like to see: HttpStreamParser: (200 response, 100 response followed by a 200 response, truncated set of headers treated as complete set of headers, 200 response with one read containing entire response body as well as headers). HttpSpdySteam: standard, push, case where all data has been read from the socket before we've tried to read the headers (If that's possible). QUIC: Same as HttpSpdyStream. URLRequest layer: Just basic HTTP, HTTPS tests should be fine. We should probably have an HTTP proxy test as well, CONNECT and non-CONNECT. Less concerned about the other proxy types (HTTP2, QUIC, SOCKS).
On 2016/07/22 18:53:50, mmenke wrote: > On 2016/07/22 18:50:09, mmenke wrote: > > > https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... > > File net/url_request/url_request.h (right): > > > > > https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... > > net/url_request/url_request.h:651: // Gets the raw header size of the request. > > On 2016/07/22 17:19:32, Blaise wrote: > > > On 2016/07/21 18:47:02, mmenke wrote: > > > > On 2016/07/21 18:10:50, mmenke wrote: > > > > > It's unclear what "raw header size" means. You should document this and > > > > rename > > > > > it (over_the_wire_response_header_size()?) > > > > > > > > > > This is over-the-wire response (Not request) header size (0 if response > > was > > > > > cached, or unable to parse headers). Some random value for HTTP/0.9 > (Good > > > > thing > > > > > we're removing support). It may or may not include protocol overhead > (For > > > H2 > > > > it > > > > > does, for QUIC it does not, it does not include HTTPS overhead). It > also > > > may > > > > > include a random number of bytes that we read after the end of an > HTTP/1.x > > > > > header, which may in fact exceed the size of the entire HTTP response. > > > > > > > > > > Hrm...This is starting to sound pretty iffy, as a web-exposed API... > > > This property contains the value of the size in bytes of the header not > > > including SSL overhead but does include H2/Quic/HTTP overhead. I am ok > > changing > > > it, but I don't know a much better name since it's not actually "over the > wire > > > size". > > > > > > > > > > > Actually, I think with H2/QUIC, it may be able to include part of the body > > as > > > > well, at least with pushed streams. Not sure about other streams. > > > > > > From what I can see in H2 we are using the value that comes from SpdyStream > > > which represents a resource. Unless content can be sent and put into this > > class > > > before headers are processed it should be safe. With Quic it appears to do > the > > > same (QuicSpdyStream). I ran some tests on a simple Go H2 server I wrote and > > it > > > appears to work as expected (tested multiple concurrent resources over same > > > socket), but have not tested on complex H2 requests. > > > > Hrm...Digging into HttpStreamParser, it looks like this may actually work, > since > > it doesn't actually return bytes read there. > > > > That having been said, I can't sign off on this without unit tests that this > > actually returns header size, and not over-the-wire bytes that we've read from > > the socket (For HTTP, in particular, we don't know header size, so we > > potentially have to read a lot more data before we learn the size of the > > headers), and updated API descriptions supporting this use case. > > > > This is something that seems like it could invisibly break, causing real > > regressions, but not failures, and no developers would notice it. If it's > worth > > doing, it's worth trying to ensure it doesn't break. > > > > Could even make an explicit API to get header size, down to the HttpStream > > layer, to even further reduce risk of regression. > > Test cases I'd like to see: > > HttpStreamParser: (200 response, 100 response followed by a 200 response, > truncated set of headers treated as complete set of headers, 200 response with > one read containing entire response body as well as headers). > HttpSpdySteam: standard, push, case where all data has been read from the > socket before we've tried to read the headers (If that's possible). > QUIC: Same as HttpSpdyStream. > > URLRequest layer: Just basic HTTP, HTTPS tests should be fine. > > We should probably have an HTTP proxy test as well, CONNECT and non-CONNECT. > Less concerned about the other proxy types (HTTP2, QUIC, SOCKS). Oh, and we should probably have HTTP auth tests, too. Not sure if we need both proxy and server auth, but we should have proxy auth across one connection and two connections.
On 2016/07/22 18:55:11, mmenke wrote: > On 2016/07/22 18:53:50, mmenke wrote: > > On 2016/07/22 18:50:09, mmenke wrote: > > > > > > https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... > > > File net/url_request/url_request.h (right): > > > > > > > > > https://codereview.chromium.org/2167853003/diff/80001/net/url_request/url_req... > > > net/url_request/url_request.h:651: // Gets the raw header size of the > request. > > > On 2016/07/22 17:19:32, Blaise wrote: > > > > On 2016/07/21 18:47:02, mmenke wrote: > > > > > On 2016/07/21 18:10:50, mmenke wrote: > > > > > > It's unclear what "raw header size" means. You should document this > and > > > > > rename > > > > > > it (over_the_wire_response_header_size()?) > > > > > > > > > > > > This is over-the-wire response (Not request) header size (0 if > response > > > was > > > > > > cached, or unable to parse headers). Some random value for HTTP/0.9 > > (Good > > > > > thing > > > > > > we're removing support). It may or may not include protocol overhead > > (For > > > > H2 > > > > > it > > > > > > does, for QUIC it does not, it does not include HTTPS overhead). It > > also > > > > may > > > > > > include a random number of bytes that we read after the end of an > > HTTP/1.x > > > > > > header, which may in fact exceed the size of the entire HTTP response. > > > > > > > > > > > > Hrm...This is starting to sound pretty iffy, as a web-exposed API... > > > > This property contains the value of the size in bytes of the header not > > > > including SSL overhead but does include H2/Quic/HTTP overhead. I am ok > > > changing > > > > it, but I don't know a much better name since it's not actually "over the > > wire > > > > size". > > > > > > > > > > > > > > Actually, I think with H2/QUIC, it may be able to include part of the > body > > > as > > > > > well, at least with pushed streams. Not sure about other streams. > > > > > > > > From what I can see in H2 we are using the value that comes from > SpdyStream > > > > which represents a resource. Unless content can be sent and put into this > > > class > > > > before headers are processed it should be safe. With Quic it appears to do > > the > > > > same (QuicSpdyStream). I ran some tests on a simple Go H2 server I wrote > and > > > it > > > > appears to work as expected (tested multiple concurrent resources over > same > > > > socket), but have not tested on complex H2 requests. > > > > > > Hrm...Digging into HttpStreamParser, it looks like this may actually work, > > since > > > it doesn't actually return bytes read there. > > > > > > That having been said, I can't sign off on this without unit tests that this > > > actually returns header size, and not over-the-wire bytes that we've read > from > > > the socket (For HTTP, in particular, we don't know header size, so we > > > potentially have to read a lot more data before we learn the size of the > > > headers), and updated API descriptions supporting this use case. > > > > > > This is something that seems like it could invisibly break, causing real > > > regressions, but not failures, and no developers would notice it. If it's > > worth > > > doing, it's worth trying to ensure it doesn't break. > > > > > > Could even make an explicit API to get header size, down to the HttpStream > > > layer, to even further reduce risk of regression. > > > > Test cases I'd like to see: > > > > HttpStreamParser: (200 response, 100 response followed by a 200 response, > > truncated set of headers treated as complete set of headers, 200 response with > > one read containing entire response body as well as headers). > > HttpSpdySteam: standard, push, case where all data has been read from the > > socket before we've tried to read the headers (If that's possible). > > QUIC: Same as HttpSpdyStream. > > > > URLRequest layer: Just basic HTTP, HTTPS tests should be fine. > > > > We should probably have an HTTP proxy test as well, CONNECT and non-CONNECT. > > Less concerned about the other proxy types (HTTP2, QUIC, SOCKS). > > Oh, and we should probably have HTTP auth tests, too. Not sure if we need both > proxy and server auth, but we should have proxy auth across one connection and > two connections. (And yes, I'm aware this is a ton of tests - but the code is completely different here for each path, and they could all regress independently, and independently of TotalReceivedBytes itself regressing, since that isn't currently checked to have this exact behavior, to the extent of my knowledge).
@mmenke, I may need some guidance on tests in netstack. I wrote a few sample tests to test standard HTTP. I am curious on where I should be placing the spdy/quic tests. Should/Can I place the spdy/quic tests here in the url_request_http_job_unittest.cc file or should I separate them into the quic/spdy directories? I looked at some similar tests and most of the ones in quic and spdy do not use URLRequest (which is where I am storing/extracting the data I need). Thanks!
On 2016/08/29 21:23:12, Blaise wrote: > @mmenke, I may need some guidance on tests in netstack. I wrote a few sample > tests to test standard HTTP. I am curious on where I should be placing the > spdy/quic tests. Should/Can I place the spdy/quic tests here in the > url_request_http_job_unittest.cc file or should I separate them into the > quic/spdy directories? I looked at some similar tests and most of the ones in > quic and spdy do not use URLRequest (which is where I am storing/extracting the > data I need). > > Thanks! Disclaimer: I'm not all that familiar with SPDY/QUIC tests. We don't use a test server for SPDY/QUIC tests, so it may be simplest to model it off of URLRequestHttpJobWithMockSocketsTest as your base (Which uses mock sockets, like the SPDY/QUIC tests, but also uses URLRequests). You'll probably need to use SpdyCreateSession (From spdy_test_util_common.h) to set up an HttpNetworkSession, and then set the network session on a TestURLRequestContext. I think that's the only additional complexity to setting things up, then you can copy the read/writes logic from the SPDY/QUIC tests, at the HttpNetworkTransaction layer. Question: Caching. For responses that come from the disk cache, and non-HTTP responses, this code will just report 0. Is that OK? For requests that go through ServiceWorker, I have no idea what this will report.
Patchset #6 (id:180001) has been deleted
Patchset #5 (id:160001) has been deleted
PTL @mmenke I did most of the tests you asked. I am not checking to see if it will work properly with PushPromsies (because I have to draw the line in the sand somewhere). The way it works right now is that spdy will consider anything sent over the same stream ID before the content is sent to be part of that request header. I thought a bit about it and seems like it is ok, we need somewhere to attribute the pushpromise-initiator header and doing it on the previous stream seems acceptable, since the client can immediately reject it and continue the resource it was trying to get in the first place. I read most of the Http2 spec and I believe you must send the header before the data (although I may be wrong about this). Quic on the other hand I don't know, but I do know that the parsing happens differently and does not consider the push promise header initialization on other streams. https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2486: 4, kServerDataStreamId1, QUIC_RST_ACKNOWLEDGEMENT, 5, 5, 1)); I was using this area as a sample and realized it was hard coding "4" instead of datastream constant id.
On 2016/08/31 23:21:40, Blaise wrote: > PTL @mmenke > > I did most of the tests you asked. I am not checking to see if it will work > properly with PushPromsies (because I have to draw the line in the sand > somewhere). The way it works right now is that spdy will consider anything sent > over the same stream ID before the content is sent to be part of that request > header. I thought a bit about it and seems like it is ok, we need somewhere to > attribute the pushpromise-initiator header and doing it on the previous stream > seems acceptable, since the client can immediately reject it and continue the > resource it was trying to get in the first place. > > I read most of the Http2 spec and I believe you must send the header before the > data (although I may be wrong about this). That's not the issue. The question is have we read any of the body from the socket/stream before the URLRequest gets notified of the headers being received, and if so, does this API report it? HttpStreamParser, for instance, does read a chunk of the body before it finishes reading the headers, because we don't even know how big the headers will be, and reading 1 byte at a time from the system is slow. Fortunately, it has logic to only report the body as being read from the network after the consumer tells it to start reading the body, even though it already has read the body over the network. So it would be completely reasonable for it to report part of the body as having been read. So the fact this works for HTTP is an implementation quirk (And hence, tests are needed to keep this behavior). It could well be SPDY does the exact same thing, and does not have that implementation quirk. Or QUIC might. > > Quic on the other hand I don't know, but I do know that the parsing happens > differently and does not consider the push promise header initialization on > other streams. > > https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... > File net/quic/chromium/quic_network_transaction_unittest.cc (right): > > https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... > net/quic/chromium/quic_network_transaction_unittest.cc:2486: 4, > kServerDataStreamId1, QUIC_RST_ACKNOWLEDGEMENT, 5, 5, 1)); > I was using this area as a sample and realized it was hard coding "4" instead of > datastream constant id.
On 2016/08/31 23:30:43, mmenke wrote: > On 2016/08/31 23:21:40, Blaise wrote: > > PTL @mmenke > > > > I did most of the tests you asked. I am not checking to see if it will work > > properly with PushPromsies (because I have to draw the line in the sand > > somewhere). The way it works right now is that spdy will consider anything > sent > > over the same stream ID before the content is sent to be part of that request > > header. I thought a bit about it and seems like it is ok, we need somewhere to > > attribute the pushpromise-initiator header and doing it on the previous stream > > seems acceptable, since the client can immediately reject it and continue the > > resource it was trying to get in the first place. > > > > I read most of the Http2 spec and I believe you must send the header before > the > > data (although I may be wrong about this). > > That's not the issue. The question is have we read any of the body from the > socket/stream before the URLRequest gets notified of the headers being received, > and if so, does this API report it? > > HttpStreamParser, for instance, does read a chunk of the body before it finishes > reading the headers, because we don't even know how big the headers will be, and > reading 1 byte at a time from the system is slow. Fortunately, it has logic to > only report the body as being read from the network after the consumer tells it > to start reading the body, even though it already has read the body over the > network. So it would be completely reasonable for it to report part of the body > as having been read. So the fact this works for HTTP is an implementation quirk > (And hence, tests are needed to keep this behavior). It could well be SPDY does > the exact same thing, and does not have that implementation quirk. Or QUIC > might. > For this case there is a test in url_request_http_job_unittest.cc::350 (TestRawHeaderSizeSuccessfullContinuiousRead). I have not seen that happen yet... The strangeness that I have seen is mostly to do with strange ordering of data/packets when in spdy/quic, but I haven't seen any issues that are not within reason (like the spdy issue I said earlier). > > > > Quic on the other hand I don't know, but I do know that the parsing happens > > differently and does not consider the push promise header initialization on > > other streams. > > > > > https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... > > File net/quic/chromium/quic_network_transaction_unittest.cc (right): > > > > > https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... > > net/quic/chromium/quic_network_transaction_unittest.cc:2486: 4, > > kServerDataStreamId1, QUIC_RST_ACKNOWLEDGEMENT, 5, 5, 1)); > > I was using this area as a sample and realized it was hard coding "4" instead > of > > datastream constant id.
The CQ bit was checked by allada@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...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
allada@chromium.org changed reviewers: + ricea@chromium.org
ricea@ can you please tell me if you see this causing any issues for you? I think rebasing the AsyncResourceHandlerTest tests would be fine here? (we can take this offline if you want clarification on what why/what this is for)
My layout tests for the PerformanceResourceTiming transferSize field pass, which is good. It implies that everything still works. What worries me is that I don't know why it still works. The only place in the renderer process that copies the value of encoded_data_length for an async response that is https://cs.chromium.org/chromium/src/content/child/web_url_loader_impl.cc?l=1017 and I'm pretty sure that code only runs if DevTools is open. Disclaimer: I looked in code search, and it may be lying to me. This makes me uncomfortable. https://codereview.chromium.org/2167853003/diff/200001/content/browser/loader... File content/browser/loader/async_resource_handler.h (right): https://codereview.chromium.org/2167853003/diff/200001/content/browser/loader... content/browser/loader/async_resource_handler.h:85: bool received_first_packet_ = 0; Nitpick: Reads do not necessary correspond to packets, and nothing else at this level talks about packets. I suggest "first_chunk_read_ = false;" https://codereview.chromium.org/2167853003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/conf/apache2-httpd-2.2.conf (right): https://codereview.chromium.org/2167853003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/conf/apache2-httpd-2.2.conf:7: MaxKeepAliveRequests 999 I'm not sure this solves the problem of Apache sending a Keep-Alive header that varies in length. In other words, I see no fundamental reason why a single content_shell can't send 900 requests over the same HTTP connection while running layout tests, at which point the Keep-Alive header will get shorter and your tests will fail. This would make them flaky in a very hard-to-reproduce manner. https://codereview.chromium.org/2167853003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources/data-xfer-resource.php (right): https://codereview.chromium.org/2167853003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources/data-xfer-resource.php:50: if ($flush_size === $wait_size) { This seems a bit convoluted. How about send_data(min($flush_size, $wait_size)); if ($sent_data_size % $flush_every_x_bytes === 0) doFlush(); if ($sent_data_size % $wait_every_x_bytes === 0) usleep($wait_duration_every_x_bytes * 1000); ?
On 2016/09/01 00:52:04, Blaise wrote: > ricea@ can you please tell me if you see this causing any issues for you? I > think rebasing the AsyncResourceHandlerTest tests would be fine here? (we can > take this offline if you want clarification on what why/what this is for) I forgot to answer your original question. It's fine to rebase the tests, as long as you add an extra test to verify that the missing encoded_data_length is set in the ResourceMsg_ReceivedResponse message.
On 2016/09/01 09:09:07, Adam Rice wrote: > On 2016/09/01 00:52:04, Blaise wrote: > > ricea@ can you please tell me if you see this causing any issues for you? I > > think rebasing the AsyncResourceHandlerTest tests would be fine here? (we can > > take this offline if you want clarification on what why/what this is for) > > I forgot to answer your original question. It's fine to rebase the tests, as > long > as you add an extra test to verify that the missing encoded_data_length is set > in the ResourceMsg_ReceivedResponse message. Sorry, looking like I won't have time to do a pass on this today.
mmenke@chromium.org changed reviewers: + rch@chromium.org
[+rch]: Mind reviewing the H2/QUIC code. Basically we want to make sure URLRequest can grab the over-the-wire header size. It's apparently difficult to test H2/QUIC at the URLRequest layer, though, so this code is doing as best it can at getting that, just using lower-layer classes. https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... net/url_request/url_request.h:651: // Gets the raw header size of the response. Worth explicitly calling out what this means? (Something like: "Over the wire header bytes, after HTTPS decryption. 0 for cached responses.") https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:343: EXPECT_FALSE(request->status().is_success()); Should expect on the error here, too (We're working on getting rid of URLRequest::status(), so also checking the exact error code will make it easier to switch to the new way of doing things) https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:375: Suggest an HTTP/0.9 test. (i.e., just a response of "Test Content", and make sure the header size is 0) https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:375: Also, should we make sure the header size is readable during the URLRequest::Delegate::OnResponseStarted / OnReceivedRedirect invocations, as opposed to only when the request is complete? I assume that's where DevTools will be grabbing it. The way we generally test such things is either stuffing them in TestDelegate (in url_request_test_util), or by subclassing it and overriding just the needed methods. https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:375: Should we have a test for the cached path? That will just return 0 (Which I assume is expected)
I commented on the QUIC tests, but they apply to SPDY as well. https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2598: TEST_P(QuicNetworkTransactionTest, TestRawHeaderSizeSuccessfullRequest) { nit: Fwiw, our tests typically don't start with "Test". Can you remove this prefix (and in the next text). This is incredibly minor, obviously :> https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2630: std::unique_ptr<URLRequest> r(quic_url_request_context.CreateRequest( nit: instead of "r" how 'about "request"? https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2641: EXPECT_EQ(31, r->raw_header_size()); Where do these magic numbers come from? Can we compute them somehow? https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2694: std::unique_ptr<URLRequest> r(quic_url_request_context.CreateRequest( ditto about "r"
PTL ricea@, rch@, mmenke@ Concerning the content tests, the encoded data length and the data length are the same size in that test, however in theory they may be different sizes if the content is encoded with a different encoding. The keep-alive header may be an issue, but there's no way to not send it in apache. This seems like the most appropriate way to handle it... I just cant have the header size change between tests and the keep alive timeout and cause issues for recycled requests. One way to handle it is to have PHP reset the keep alive connections, but I don't think that's a good solution either. https://codereview.chromium.org/2167853003/diff/200001/content/browser/loader... File content/browser/loader/async_resource_handler.h (right): https://codereview.chromium.org/2167853003/diff/200001/content/browser/loader... content/browser/loader/async_resource_handler.h:85: bool received_first_packet_ = 0; On 2016/09/01 08:57:28, Adam Rice wrote: > Nitpick: Reads do not necessary correspond to packets, and nothing else at this > level talks about packets. > > I suggest "first_chunk_read_ = false;" Done. https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2598: TEST_P(QuicNetworkTransactionTest, TestRawHeaderSizeSuccessfullRequest) { On 2016/09/06 19:54:51, Ryan Hamilton wrote: > nit: Fwiw, our tests typically don't start with "Test". Can you remove this > prefix (and in the next text). > > This is incredibly minor, obviously :> Done. https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2630: std::unique_ptr<URLRequest> r(quic_url_request_context.CreateRequest( On 2016/09/06 19:54:51, Ryan Hamilton wrote: > nit: instead of "r" how 'about "request"? Done. https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2641: EXPECT_EQ(31, r->raw_header_size()); On 2016/09/06 19:54:51, Ryan Hamilton wrote: > Where do these magic numbers come from? Can we compute them somehow? Done. https://codereview.chromium.org/2167853003/diff/200001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2694: std::unique_ptr<URLRequest> r(quic_url_request_context.CreateRequest( On 2016/09/06 19:54:51, Ryan Hamilton wrote: > ditto about "r" Done. https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... net/url_request/url_request.h:651: // Gets the raw header size of the response. On 2016/09/06 18:55:00, mmenke wrote: > Worth explicitly calling out what this means? (Something like: "Over the wire > header bytes, after HTTPS decryption. 0 for cached responses.") Done. https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:343: EXPECT_FALSE(request->status().is_success()); On 2016/09/06 18:55:00, mmenke wrote: > Should expect on the error here, too (We're working on getting rid of > URLRequest::status(), so also checking the exact error code will make it easier > to switch to the new way of doing things) Done. https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:375: On 2016/09/06 18:55:00, mmenke wrote: > Suggest an HTTP/0.9 test. (i.e., just a response of "Test Content", and make > sure the header size is 0) I was opting out of doing this kind of test since we are trying to phase 0.9 out. I would rather not encourage any developers use 0.9. https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:375: On 2016/09/06 18:55:00, mmenke wrote: > Should we have a test for the cached path? That will just return 0 (Which I > assume is expected) I can do a cached path, however, in our case it's not important. The data about the resource is very subjective based on many criteria like where the cached object was returned (eg: memory, disk, etc...). Many/most developers turn cache off when devtools is open anyway to get more accurate readings. https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:375: On 2016/09/06 18:55:00, mmenke wrote: > Also, should we make sure the header size is readable during the > URLRequest::Delegate::OnResponseStarted / OnReceivedRedirect invocations, as > opposed to only when the request is complete? I assume that's where DevTools > will be grabbing it. The way we generally test such things is either stuffing > them in TestDelegate (in url_request_test_util), or by subclassing it and > overriding just the needed methods. We have this kind of test in our tests: third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-data-length.html https://codereview.chromium.org/2167853003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/conf/apache2-httpd-2.2.conf (right): https://codereview.chromium.org/2167853003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/conf/apache2-httpd-2.2.conf:7: MaxKeepAliveRequests 999 On 2016/09/01 08:57:28, Adam Rice wrote: > I'm not sure this solves the problem of Apache sending a Keep-Alive header that > varies in length. > > In other words, I see no fundamental reason why a single content_shell can't > send 900 requests over the same HTTP connection while running layout tests, at > which point the Keep-Alive header will get shorter and your tests will fail. > This would make them flaky in a very hard-to-reproduce manner. A quick search found apx 4.2k (some may have multiple requests, but it also shards the tests) tests that use HTTP, although I do agree this should be avoided, there is no easy solution. I increased the number to help mitigate this concern. https://codereview.chromium.org/2167853003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources/data-xfer-resource.php (right): https://codereview.chromium.org/2167853003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources/data-xfer-resource.php:50: if ($flush_size === $wait_size) { On 2016/09/01 08:57:28, Adam Rice wrote: > This seems a bit convoluted. How about > > send_data(min($flush_size, $wait_size)); > if ($sent_data_size % $flush_every_x_bytes === 0) > doFlush(); > if ($sent_data_size % $wait_every_x_bytes === 0) > usleep($wait_duration_every_x_bytes * 1000); > > ? Done.
https://codereview.chromium.org/2167853003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/conf/apache2-httpd-2.2.conf (right): https://codereview.chromium.org/2167853003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/conf/apache2-httpd-2.2.conf:7: MaxKeepAliveRequests 999 On 2016/09/07 06:01:12, Blaise wrote: > On 2016/09/01 08:57:28, Adam Rice wrote: > > I'm not sure this solves the problem of Apache sending a Keep-Alive header > that > > varies in length. > > > > In other words, I see no fundamental reason why a single content_shell can't > > send 900 requests over the same HTTP connection while running layout tests, at > > which point the Keep-Alive header will get shorter and your tests will fail. > > This would make them flaky in a very hard-to-reproduce manner. > > A quick search found apx 4.2k (some may have multiple requests, but it also > shards the tests) tests that use HTTP, although I do agree this should be > avoided, there is no easy solution. I increased the number to help mitigate this > concern. MaxKeepAliveRequests 0 seems to work. "0" means unlimited here. This makes Apache stop including the count in the Keep-Alive: header. Since Apache is stopped at the end of the layout tests anyway, there should be no adverse affects.
https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:375: On 2016/09/07 06:01:12, Blaise wrote: > On 2016/09/06 18:55:00, mmenke wrote: > > Should we have a test for the cached path? That will just return 0 (Which I > > assume is expected) > > I can do a cached path, however, in our case it's not important. The data about > the resource is very subjective based on many criteria like where the cached > object was returned (eg: memory, disk, etc...). Many/most developers turn cache > off when devtools is open anyway to get more accurate readings. My concern is just that we don't return nonsense. https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:375: On 2016/09/07 06:01:12, Blaise wrote: > On 2016/09/06 18:55:00, mmenke wrote: > > Suggest an HTTP/0.9 test. (i.e., just a response of "Test Content", and make > > sure the header size is 0) > > I was opting out of doing this kind of test since we are trying to phase 0.9 > out. I would rather not encourage any developers use 0.9. Unfortunately, I don't think we'll be able to completely remove it. The firmware of some home routers use it (Probably due to bugs, rather than an intentional behavior), and I'm very reluctant to break them, though I suppose if we had buy-in from other browsers... https://codereview.chromium.org/2167853003/diff/220001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:295: EXPECT_EQ((int)content_data.size(), C-style casts are forbidden. static_casts. Goes for all this code. https://codereview.chromium.org/2167853003/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:356: EXPECT_EQ(URLRequestStatus::SUCCESS, request->status().status()); On 2016/09/07 06:01:12, Blaise wrote: > On 2016/09/06 18:55:00, mmenke wrote: > > Should expect on the error here, too (We're working on getting rid of > > URLRequest::status(), so also checking the exact error code will make it > easier > > to switch to the new way of doing things) > > Done. Sorry, I meant EXPECT_EQ(ERR_<whatever>, request->status().error());
looks good minus a few nits. https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2622: TestDelegate d; nit: "delegate" instead of "d". https://google.github.io/styleguide/cppguide.html#General_Naming_Rules https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2639: EXPECT_GT(request->GetTotalSentBytes(), 0); nit: I'm accustomed to seeing EXPECT written (expected, actual), which would suggest putting the 0 first. (and elsewhere) https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2645: EXPECT_EQ((int)expected_raw_header_response_size, request->raw_header_size()); nit: static_cast instead of (int). https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2692: TestDelegate d; ditto https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2715: EXPECT_EQ((int)expected_raw_header_response_size, request->raw_header_size()); ditto https://codereview.chromium.org/2167853003/diff/220001/net/spdy/spdy_network_... File net/spdy/spdy_network_transaction_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/220001/net/spdy/spdy_network_... net/spdy/spdy_network_transaction_unittest.cc:2131: TestDelegate d; ditto https://codereview.chromium.org/2167853003/diff/220001/net/spdy/spdy_network_... net/spdy/spdy_network_transaction_unittest.cc:2146: EXPECT_GT(request->GetTotalSentBytes(), 0); ditto https://codereview.chromium.org/2167853003/diff/220001/net/spdy/spdy_network_... net/spdy/spdy_network_transaction_unittest.cc:2204: TestDelegate d; ditto https://codereview.chromium.org/2167853003/diff/220001/net/spdy/spdy_network_... net/spdy/spdy_network_transaction_unittest.cc:2219: EXPECT_GT(request->GetTotalSentBytes(), 0); ditto
PTL ricea@ mmenke@ and rch@ https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/200001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:375: On 2016/09/07 15:18:21, mmenke wrote: > On 2016/09/07 06:01:12, Blaise wrote: > > On 2016/09/06 18:55:00, mmenke wrote: > > > Suggest an HTTP/0.9 test. (i.e., just a response of "Test Content", and > make > > > sure the header size is 0) > > > > I was opting out of doing this kind of test since we are trying to phase 0.9 > > out. I would rather not encourage any developers use 0.9. > > Unfortunately, I don't think we'll be able to completely remove it. The > firmware of some home routers use it (Probably due to bugs, rather than an > intentional behavior), and I'm very reluctant to break them, though I suppose if > we had buy-in from other browsers... What if we refuse to set the header size if the version < 1.0? I feel strongly that if any developer trying to get reliable information about 0.9 (or is using any 0.9) we should simply stand by the argument of "we cannot maintain reliable information on 0.9". In devtool's case we do not need to worry about this since we are subtracting it from the next chunk's data length, as long as it returns 0. https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2622: TestDelegate d; On 2016/09/07 16:54:50, Ryan Hamilton wrote: > nit: "delegate" instead of "d". > > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules The reason I was naming these variables simple is because other tests do the same. I have changed them, but maybe we should change the other areas as well? https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2639: EXPECT_GT(request->GetTotalSentBytes(), 0); On 2016/09/07 16:54:50, Ryan Hamilton wrote: > nit: I'm accustomed to seeing EXPECT written (expected, actual), which would > suggest putting the 0 first. (and elsewhere) Done. https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2645: EXPECT_EQ((int)expected_raw_header_response_size, request->raw_header_size()); On 2016/09/07 16:54:50, Ryan Hamilton wrote: > nit: static_cast instead of (int). Done. https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2692: TestDelegate d; On 2016/09/07 16:54:50, Ryan Hamilton wrote: > ditto Done. https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2715: EXPECT_EQ((int)expected_raw_header_response_size, request->raw_header_size()); On 2016/09/07 16:54:50, Ryan Hamilton wrote: > ditto Done. https://codereview.chromium.org/2167853003/diff/220001/net/spdy/spdy_network_... File net/spdy/spdy_network_transaction_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/220001/net/spdy/spdy_network_... net/spdy/spdy_network_transaction_unittest.cc:2131: TestDelegate d; On 2016/09/07 16:54:50, Ryan Hamilton wrote: > ditto Done. https://codereview.chromium.org/2167853003/diff/220001/net/spdy/spdy_network_... net/spdy/spdy_network_transaction_unittest.cc:2146: EXPECT_GT(request->GetTotalSentBytes(), 0); On 2016/09/07 16:54:50, Ryan Hamilton wrote: > ditto Done. https://codereview.chromium.org/2167853003/diff/220001/net/spdy/spdy_network_... net/spdy/spdy_network_transaction_unittest.cc:2204: TestDelegate d; On 2016/09/07 16:54:50, Ryan Hamilton wrote: > ditto Done. https://codereview.chromium.org/2167853003/diff/220001/net/spdy/spdy_network_... net/spdy/spdy_network_transaction_unittest.cc:2219: EXPECT_GT(request->GetTotalSentBytes(), 0); On 2016/09/07 16:54:50, Ryan Hamilton wrote: > ditto Done. https://codereview.chromium.org/2167853003/diff/220001/net/url_request/url_re... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:295: EXPECT_EQ((int)content_data.size(), On 2016/09/07 15:18:21, mmenke wrote: > C-style casts are forbidden. static_casts. Goes for all this code. Done. https://codereview.chromium.org/2167853003/diff/220001/net/url_request/url_re... net/url_request/url_request_http_job_unittest.cc:356: EXPECT_EQ(URLRequestStatus::SUCCESS, request->status().status()); On 2016/09/07 15:18:21, mmenke wrote: > On 2016/09/07 06:01:12, Blaise wrote: > > On 2016/09/06 18:55:00, mmenke wrote: > > > Should expect on the error here, too (We're working on getting rid of > > > URLRequest::status(), so also checking the exact error code will make it > > easier > > > to switch to the new way of doing things) > > > > Done. > > Sorry, I meant EXPECT_EQ(ERR_<whatever>, request->status().error()); Done.
The CQ bit was checked by allada@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
lgtm https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... net/quic/chromium/quic_network_transaction_unittest.cc:2622: TestDelegate d; On 2016/09/07 18:14:57, Blaise wrote: > On 2016/09/07 16:54:50, Ryan Hamilton wrote: > > nit: "delegate" instead of "d". > > > > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules > > The reason I was naming these variables simple is because other tests do the > same. I have changed them, but maybe we should change the other areas as well? That's surprising! I don't see single character variables in this file. Can you point me at an example. I'll definitely make sure they get fixed.
The CQ bit was checked by allada@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...
On 2016/09/07 18:39:40, Ryan Hamilton wrote: > lgtm > > https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... > File net/quic/chromium/quic_network_transaction_unittest.cc (right): > > https://codereview.chromium.org/2167853003/diff/220001/net/quic/chromium/quic... > net/quic/chromium/quic_network_transaction_unittest.cc:2622: TestDelegate d; > On 2016/09/07 18:14:57, Blaise wrote: > > On 2016/09/07 16:54:50, Ryan Hamilton wrote: > > > nit: "delegate" instead of "d". > > > > > > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules > > > > The reason I was naming these variables simple is because other tests do the > > same. I have changed them, but maybe we should change the other areas as well? > > That's surprising! I don't see single character variables in this file. Can you > point me at an example. I'll definitely make sure they get fixed. Not in that file, there's quite a few here in this file: https://cs.chromium.org/chromium/src/net/spdy/spdy_network_transaction_unitte... and https://cs.chromium.org/chromium/src/net/url_request/url_request_unittest.cc?...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #8 (id:260001) has been deleted
The CQ bit was checked by allada@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #9 (id:290001) has been deleted
Patchset #8 (id:280001) has been deleted
The CQ bit was checked by allada@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by allada@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...
net/ LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2167853003/diff/330001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-data-length.html (right): https://codereview.chromium.org/2167853003/diff/330001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-data-length.html:106: function sendRequest(url) { { on next line https://codereview.chromium.org/2167853003/diff/330001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-data-length.html:112: function printResults() { { on next line https://codereview.chromium.org/2167853003/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/NetworkResourcesData.h (right): https://codereview.chromium.org/2167853003/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/NetworkResourcesData.h:134: size_t dataLength() const; Let's move this back to private. https://codereview.chromium.org/2167853003/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/NetworkResourcesData.h:161: int m_rawHeaderSize; Initialize to 0 in constructor.
done. https://codereview.chromium.org/2167853003/diff/330001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-data-length.html (right): https://codereview.chromium.org/2167853003/diff/330001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-data-length.html:106: function sendRequest(url) { On 2016/09/08 23:39:49, dgozman wrote: > { on next line Done. https://codereview.chromium.org/2167853003/diff/330001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-data-length.html:112: function printResults() { On 2016/09/08 23:39:49, dgozman wrote: > { on next line Done. https://codereview.chromium.org/2167853003/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/NetworkResourcesData.h (right): https://codereview.chromium.org/2167853003/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/NetworkResourcesData.h:134: size_t dataLength() const; On 2016/09/08 23:39:50, dgozman wrote: > Let's move this back to private. Done. https://codereview.chromium.org/2167853003/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/NetworkResourcesData.h:161: int m_rawHeaderSize; On 2016/09/08 23:39:50, dgozman wrote: > Initialize to 0 in constructor. Done.
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, rch@chromium.org, dgozman@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2167853003/#ps350001 (title: "changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:350001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Always report encodedDataLength in Network.ResponseReceived. Currently, we only send encodedDataLength for redirects. However, we already know how much we read when sending ResponseReceived. This will improve reporting for failed requests, as they don't get loadingFinished with full encodedDataLength. Previous CL: https://codereview.chromium.org/2101073002 BUG=622018 ========== to ========== [DevTools] Always report encodedDataLength in Network.ResponseReceived. Currently, we only send encodedDataLength for redirects. However, we already know how much we read when sending ResponseReceived. This will improve reporting for failed requests, as they don't get loadingFinished with full encodedDataLength. Previous CL: https://codereview.chromium.org/2101073002 BUG=622018 Committed: https://crrev.com/71b2efb2261233d22779f8ee192b81e7a942fe2b Cr-Commit-Position: refs/heads/master@{#417506} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/71b2efb2261233d22779f8ee192b81e7a942fe2b Cr-Commit-Position: refs/heads/master@{#417506}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2167853003/diff/350001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources/data-xfer-resource.php (right): https://codereview.chromium.org/2167853003/diff/350001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources/data-xfer-resource.php:51: $send_size = min($flish_size, $wait_size); This is probably supposed to be flush_size, not flish_size (also 2 lines below). Since things don't seem to explode without this, do you really need this code? (I happened to look through apache error logs for something else and noticed [Thu May 11 09:05:00.228761 2017] [:error] [pid 825] [client 127.0.0.1:35972] PHP Notice: Undefined variable: flish_size in /b/s/w/ir/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources/data-xfer-resource.php on line 51, referer: http://127.0.0.1:8000/inspector-protocol/network-data-length.html ) |
