Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2092993002:
Browser process changes for Resource Timing sizes. (Closed)
|
Created:
4 years, 6 months ago by Adam Rice Modified:
4 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, loading-reviews_chromium.org, mmenke, Randy Smith (Not in Mondays) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBrowser process changes for Resource Timing sizes.
This is part of the changes to add the size fields to the
PerformanceResourceTiming API.
See the design doc at
https://docs.google.com/document/d/1ckL-rKLFRsdI4nn1golvQ6I1zRIvxgFkDXMrZb8KduY/edit
These are the browser-side changes to pass the "encodedBodySize" field to the
renderer. The field is named encoded_body_length in the //content code for
consistency with the existing encoded_data_length field.
For async resource fetches the value is passed one chunk at a time in the
ResourceMsg_InlinedDataChunkReceived and ResourceMsg_DataReceived IPCs. For sync
XHR it is passed in the ResourceResponseInfo struct.
BUG=467945
Committed: https://crrev.com/e7b920b234f45614806cc4467e3e602720839e1e
Cr-Commit-Position: refs/heads/master@{#405015}
Patch Set 1 #Patch Set 2 : Fix compilation of resource_dispatcher_unittest.cc #
Total comments: 6
Patch Set 3 : Add tests for AsyncResourceHandler #Patch Set 4 : Add missing explicit keyword #
Total comments: 11
Patch Set 5 : Use ResourceRequestInfo::AllocateForTesting(). Small code and comment cleanups. #
Total comments: 10
Patch Set 6 : Stop using AllocateForTesting() #
Total comments: 23
Patch Set 7 : AsyncResourceHandlerTest cleanups from kinuko review #Patch Set 8 : Fix content_features.cc header include #
Total comments: 12
Patch Set 9 : Nitpick fixes #Patch Set 10 : Rebase #Patch Set 11 : Fix leak and CHECK error #Patch Set 12 : Fix base::FeatureList DCHECK again #
Total comments: 3
Patch Set 13 : Method renames #Messages
Total messages: 61 (30 generated)
ricea@chromium.org changed reviewers: + ksakamoto@chromium.org
PTAL
non-owner lgtm
ricea@chromium.org changed reviewers: + kinuko@chromium.org
kinuko, do you have time to do the OWNERS review for this?
lgtm.
The CQ bit was checked by ricea@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2092993002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ricea@chromium.org changed reviewers: + mkwst@chromium.org
+mkwst for security review of resource_messages.h
IPC LGTM, but I think this CL would benefit from additional testing. https://codereview.chromium.org/2092993002/diff/20001/content/browser/loader/... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2092993002/diff/20001/content/browser/loader/... content/browser/loader/async_resource_handler.cc:76: // *cached_length to new_length. "length" is a weird-looking word. Especially when repeated so many times in close proximity. :) https://codereview.chromium.org/2092993002/diff/20001/content/child/resource_... File content/child/resource_dispatcher_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/20001/content/child/resource_... content/child/resource_dispatcher_unittest.cc:279: request_id, 0, data.length(), data.length(), data.length()))); It surprises me that this is the only test change. Is it possible to write some tests that verify that the correct information is being handed down? And that the (trivial) change to the implementation of `CalculateEncodedDataLengthToReport` didn't change the result?
https://codereview.chromium.org/2092993002/diff/20001/content/public/common/r... File content/public/common/resource_response_info.cc (right): https://codereview.chromium.org/2092993002/diff/20001/content/public/common/r... content/public/common/resource_response_info.cc:16: encoded_body_length(0), This being 0 by default while others are -1 comes from the spec? It feels a bit inconsistent... does it make sense to keep these -1 but return 0 at the final ResponseTiming API implementation in Blink?
I thought I sent these comments but it seems not. Sorry for the delay. https://codereview.chromium.org/2092993002/diff/20001/content/browser/loader/... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2092993002/diff/20001/content/browser/loader/... content/browser/loader/async_resource_handler.cc:76: // *cached_length to new_length. On 2016/06/24 10:49:20, Mike West wrote: > "length" is a weird-looking word. Especially when repeated so many times in > close proximity. :) You could try filing a bug against the English language. :-) In the meantime, I realised this function really has nothing to do with length and so was able to make it a lot more concise. https://codereview.chromium.org/2092993002/diff/20001/content/child/resource_... File content/child/resource_dispatcher_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/20001/content/child/resource_... content/child/resource_dispatcher_unittest.cc:279: request_id, 0, data.length(), data.length(), data.length()))); On 2016/06/24 10:49:20, Mike West wrote: > It surprises me that this is the only test change. Is it possible to write some > tests that verify that the correct information is being handed down? And that > the (trivial) change to the implementation of > `CalculateEncodedDataLengthToReport` didn't change the result? AsyncResourceHandler had no tests. I have added some. I also added a test for the ResourceMsg_InlinedDataChunkReceived IPC to ResourceDispatcher. https://codereview.chromium.org/2092993002/diff/20001/content/public/common/r... File content/public/common/resource_response_info.cc (right): https://codereview.chromium.org/2092993002/diff/20001/content/public/common/r... content/public/common/resource_response_info.cc:16: encoded_body_length(0), On 2016/06/28 15:10:17, kinuko wrote: > This being 0 by default while others are -1 comes from the spec? It feels a bit > inconsistent... does it make sense to keep these -1 but return 0 at the final > ResponseTiming API implementation in Blink? Actually, it's only 0 in Javascript when we can't supply it due to the cross-origin restriction. So the default value shouldn't matter. I have changed it to -1.
https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:58: // Should be larger than kInlinedLeadingChunkSize nit: end sentence with period (here and below) https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:81: static GURL test_url_large_length() { return GURL("test:large"); } Not sure why these need to be static methods, just constants would be enough? https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:94: if (spec == test_url_short_length().spec()) { nit: == operator just works for GURLs, no need to get spec() https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:128: class TestJobProtocolHandler While writing up all these test harness is nice and might be useful, I'm not fully sure if we really need all these ProtocolHandlers, URLRequestJob, ResourceLoader and LoaderDelegate to test the code we're adding in the main CL? Let me clarify... what we basically want to test is multiple calls of AsyncResourceHnadler::OnReadCompleted() and the outputs to the message filter, right? https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:216: ResourceRequestInfoImpl* info = new ResourceRequestInfoImpl( Use ResourceRequestInfo::AllocateForTesting ?
https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:58: // Should be larger than kInlinedLeadingChunkSize On 2016/07/08 05:57:05, kinuko wrote: > nit: end sentence with period (here and below) Done. https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:81: static GURL test_url_large_length() { return GURL("test:large"); } On 2016/07/08 05:57:05, kinuko wrote: > Not sure why these need to be static methods, just constants would be enough? Sorry, I was copying URLRequestTestJob without thinking. I have changed them to constants. https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:94: if (spec == test_url_short_length().spec()) { On 2016/07/08 05:57:05, kinuko wrote: > nit: == operator just works for GURLs, no need to get spec() Done. https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:128: class TestJobProtocolHandler On 2016/07/08 05:57:05, kinuko wrote: > While writing up all these test harness is nice and might be useful, I'm not > fully sure if we really need all these ProtocolHandlers, URLRequestJob, > ResourceLoader and LoaderDelegate to test the code we're adding in the main CL? > > Let me clarify... what we basically want to test is multiple calls of > AsyncResourceHnadler::OnReadCompleted() and the outputs to the message filter, > right? A real URLRequest is needed to return non-zero values from GetTotalReceivedBytes() and GetRawBodyBytes(). We need to really read the response data in order for those values to be non-zero. A ResourceLoader provides a convenient and correct way to drive the URLRequest. A ResourceLoaderDelegate is needed to create a ResourceLoader. It is necessary to subclass URLRequestTestJob in order to return custom headers and data. We also need our URLRequestJob to have an implementation of GetTotalReceivedBytes(), which URLRequestTestJob doesn't provide. A ProtocolHandler is needed to register a URLRequestJob. https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:216: ResourceRequestInfoImpl* info = new ResourceRequestInfoImpl( On 2016/07/08 05:57:05, kinuko wrote: > Use ResourceRequestInfo::AllocateForTesting ? Done.
I'll take further look. While having a test is great in general I still feel it's a bit overkilling but I'm not fully sure. Mike: if you have any comments regarding the tests I'd like to hear that too. https://codereview.chromium.org/2092993002/diff/80001/content/browser/loader/... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:93: // responses for our tests. While this description is very useful for reviewing code it's probably too lengthy / verbose to have in the code (and it adds some maintenance cost too when others want to add more code to this class). Maybe just: "This test job is to add Content-Length header and implement GetTotalReceivedBytes()" is enough. btw as for GetTotalReceivedBytes() we can just implement that in URLRequestTestJob, right? https://codereview.chromium.org/2092993002/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:136: // A ProtocolHandler is necessary to set TestJob as the URLRequestJob. I understand you added this as a follow-up for my review, but I don't think this comment's necessary, please remove (it's obvious we need this as far as we need custom job) https://codereview.chromium.org/2092993002/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:140: // URLRequestJobFactory::ProtocolHandler implementation: nit: in this case it's obvious too, I don't think this comment's necessary https://codereview.chromium.org/2092993002/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:303: "must be updated."); I feel these static_asserts are a bit too much. When test breaks we can look into... that's how most tests are written, aren't they? https://codereview.chromium.org/2092993002/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:306: if (!ResourceMsg_DataReceived::Read(msg, params.get())) Is checking the return value that useful here?
https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/60001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:216: ResourceRequestInfoImpl* info = new ResourceRequestInfoImpl( On 2016/07/08 07:45:25, Adam Rice wrote: > On 2016/07/08 05:57:05, kinuko wrote: > > Use ResourceRequestInfo::AllocateForTesting ? > > Done. I failed to spot that AllocateForTesting() doesn't let us set the ResourceMessageFilter field, and so it broke the test. Reverted. https://codereview.chromium.org/2092993002/diff/80001/content/browser/loader/... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:93: // responses for our tests. On 2016/07/08 09:39:28, kinuko wrote: > While this description is very useful for reviewing code it's probably too > lengthy / verbose to have in the code (and it adds some maintenance cost too > when others want to add more code to this class). > > Maybe just: "This test job is to add Content-Length header and implement > GetTotalReceivedBytes()" is enough. Done. > btw as for GetTotalReceivedBytes() we can just implement that in > URLRequestTestJob, right? I think this is worthwhile but I'd rather do it in a separate CL. https://codereview.chromium.org/2092993002/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:136: // A ProtocolHandler is necessary to set TestJob as the URLRequestJob. On 2016/07/08 09:39:28, kinuko wrote: > I understand you added this as a follow-up for my review, but I don't think this > comment's necessary, please remove (it's obvious we need this as far as we need > custom job) Done. https://codereview.chromium.org/2092993002/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:140: // URLRequestJobFactory::ProtocolHandler implementation: On 2016/07/08 09:39:28, kinuko wrote: > nit: in this case it's obvious too, I don't think this comment's necessary Done. https://codereview.chromium.org/2092993002/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:303: "must be updated."); On 2016/07/08 09:39:28, kinuko wrote: > I feel these static_asserts are a bit too much. When test breaks we can look > into... that's how most tests are written, aren't they? I wanted to save debugging time. But since new arguments will usually be added to the end, it may waste more time than it saves. Removed. https://codereview.chromium.org/2092993002/diff/80001/content/browser/loader/... content/browser/loader/async_resource_handler_unittest.cc:306: if (!ResourceMsg_DataReceived::Read(msg, params.get())) On 2016/07/08 09:39:28, kinuko wrote: > Is checking the return value that useful here? As far I can tell it will only catch bugs in the IPC implementation, which is not the purpose of this test. Removed.
On 2016/07/08 at 09:39:28, kinuko wrote: > I'll take further look. While having a test is great in general I still feel it's a bit overkilling but I'm not fully sure. > > Mike: if you have any comments regarding the tests I'd like to hear that too. If I'd known that you'd have to do this much setup in order to build up the context to test this CL's methods, I would have punted on it. :) Now that you've done that work, however, I don't think it's overkill to land it (and to eventually build up tests for other pieces).. The test LGTM (and I appreciate the refactored implementation as well, it's cleaner!). https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:219: ResourceRequestInfoImpl* info = new ResourceRequestInfoImpl( Nit: Wrap this in `// clang format off` at the top and `// clang format on` at the bottom; otherwise `git cl format` will make you sad.
https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:219: ResourceRequestInfoImpl* info = new ResourceRequestInfoImpl( On 2016/07/08 13:14:37, Mike West wrote: > Nit: Wrap this in `// clang format off` at the top and `// clang format on` at > the bottom; otherwise `git cl format` will make you sad. Actually, the comments on each line are sufficient to stop clang-format from trying to be clever.
Thanks Mike. Ok then let's land this with the additional test. I think we can further simplify / possibly improve the test code. https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:86: } Why can't we use simple method like std::string(target_size, 'a')? https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:95: // URLRequestJob implementation: Should we have a TODO to implement this in URLRequestTestJob? https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:104: void StartAsync() override { I think it's easier to extend / more readable if we use the version of net::URLRequestTestJob ctor which takes response_headers and response_data rather than having multiple test-dependent cases in this StartAsync. This class's ctor can take take target_size and can be more modularized then. I imagine something like following: TestJob(URLRequest*, NetworkDelegate*, size_t response_data_size) : net::URLRequestTestJob(request, network_delegate, GenerateHeader(response_data_size), GenerateData(response_data_size), true /* auto_advance */) {} GenerateHeader can use a simple Stringf format template and returns the raw header string (it can be one-line StringPrintf method + string constant then). We can have a static method to do CreateJob, and TestJobProtocolHandler's ctor can take base::Callback that calls the method with target_size parameter. Then we can remove all the URL/size constants above and embed them in each test code... that could possibly actually reduce the # of lines too. Wdyt? https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:191: IPC_END_MESSAGE_MAP() Isn't it more compact / easier to read if we just write in Send(): // Unpickle the base::SharedMemoryHandle to warnings about // "MessageAttachmentSet destroyed with unconsumed descriptors". if (message->type() == ResourceMsg_SetDataBuffer::ID) { ResourceMsg_SetDataBuffer::Param params; ResourceMsg_SetDataBuffer::Read(message, ¶ms); } ? https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:219: ResourceRequestInfoImpl* info = new ResourceRequestInfoImpl( Note that ResourceRequestInfo::AllocateForTesting method has an explicit comment like 'NOTE: Add more parameters if you need to initialize other fields', so I think we can try adding the filter thing to it too. (No other test code seems to directly use this ctor) (As it requires more changes in other test code it can be done in a separate CL too) https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:259: void WaitForFinish() { finish_waiter_.Run(); } StartRequest and WaitForFinish are called only from CreateStartAndWait, I don't feel we need to have these methods separately https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:274: base::RunLoop finish_waiter_; Looks like all fields other than filter_ can be actually private? https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:277: class TrivialResourceLoaderDelegate : public ResourceLoaderDelegate { I think AsyncResourceHandlerTest can directly implement ResourceLoaderDelegate? Then we can remove 'fixture_' indirection in DidFinishLoading and can remove loader_delegate_ field from the test class. https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:340: ASSERT_TRUE(params); I have a feeling that these two lines can be replaced with ResourceMessage_DataReceived::Param params; ASSERT_TRUE(ResourceMsg_DataReceived::Read(messages[2].get, ¶ms); here and following. Then we can remove UnpackX methods at all. https://codereview.chromium.org/2092993002/diff/100001/content/child/resource... File content/child/resource_dispatcher_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/100001/content/child/resource... content/child/resource_dispatcher_unittest.cc:34: #include "content/public/common/content_features.cc" content_features.h ?
https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:86: } On 2016/07/11 02:35:24, kinuko wrote: > Why can't we use simple method like std::string(target_size, 'a')? Oh. Yes. Sorry. Done. https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:95: // URLRequestJob implementation: On 2016/07/11 02:35:25, kinuko wrote: > Should we have a TODO to implement this in URLRequestTestJob? Done. https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:104: void StartAsync() override { On 2016/07/11 02:35:25, kinuko wrote: > I think it's easier to extend / more readable if we use the version of > net::URLRequestTestJob ctor which takes response_headers and response_data > rather than having multiple test-dependent cases in this StartAsync. This > class's ctor can take take target_size and can be more modularized then. I > imagine something like following: > > TestJob(URLRequest*, NetworkDelegate*, size_t response_data_size) > : net::URLRequestTestJob(request, network_delegate, > GenerateHeader(response_data_size), > GenerateData(response_data_size), > true /* auto_advance */) {} > > GenerateHeader can use a simple Stringf format template and returns the raw > header string (it can be one-line StringPrintf method + string constant then). > > We can have a static method to do CreateJob, and TestJobProtocolHandler's ctor > can take base::Callback that calls the method with target_size parameter. > > Then we can remove all the URL/size constants above and embed them in each test > code... that could possibly actually reduce the # of lines too. Wdyt? That is simpler. Done. https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:191: IPC_END_MESSAGE_MAP() On 2016/07/11 02:35:25, kinuko wrote: > Isn't it more compact / easier to read if we just write in Send(): > > // Unpickle the base::SharedMemoryHandle to warnings about > // "MessageAttachmentSet destroyed with unconsumed descriptors". > if (message->type() == ResourceMsg_SetDataBuffer::ID) { > ResourceMsg_SetDataBuffer::Param params; > ResourceMsg_SetDataBuffer::Read(message, ¶ms); > } > > ? I didn't think that would work. I should have tried it. It does work. Thank you. Done. https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:219: ResourceRequestInfoImpl* info = new ResourceRequestInfoImpl( On 2016/07/11 02:35:24, kinuko wrote: > Note that ResourceRequestInfo::AllocateForTesting method has an explicit comment > like 'NOTE: Add more parameters if you need to initialize other fields', so I > think we can try adding the filter thing to it too. (No other test code seems to > directly use this ctor) > > (As it requires more changes in other test code it can be done in a separate CL > too) ResourceRequestInfo::AllocateForTesting() only sets fields which are exposed by ResourceRequestInfo. This doesn't include the ResourceContext field. This makes sense from an encapsulation point of view. I thought about adding a ResourceRequestInfoImpl::AllocateForTesting() function, but I concluded it would be better just to simplify the ResourceRequestInfoImpl constructor. https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:259: void WaitForFinish() { finish_waiter_.Run(); } On 2016/07/11 02:35:25, kinuko wrote: > StartRequest and WaitForFinish are called only from CreateStartAndWait, I don't > feel we need to have these methods separately Done. https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:274: base::RunLoop finish_waiter_; On 2016/07/11 02:35:24, kinuko wrote: > Looks like all fields other than filter_ can be actually private? Done. https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:277: class TrivialResourceLoaderDelegate : public ResourceLoaderDelegate { On 2016/07/11 02:35:24, kinuko wrote: > I think AsyncResourceHandlerTest can directly implement ResourceLoaderDelegate? > Then we can remove 'fixture_' indirection in DidFinishLoading and can remove > loader_delegate_ field from the test class. Done. https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:340: ASSERT_TRUE(params); On 2016/07/11 02:35:24, kinuko wrote: > I have a feeling that these two lines can be replaced with > > ResourceMessage_DataReceived::Param params; > ASSERT_TRUE(ResourceMsg_DataReceived::Read(messages[2].get, ¶ms); > > here and following. Then we can remove UnpackX methods at all. Read() doesn't check the message type, so I had to add ASSERTs for that. Apart from that, this works. https://codereview.chromium.org/2092993002/diff/100001/content/child/resource... File content/child/resource_dispatcher_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/100001/content/child/resource... content/child/resource_dispatcher_unittest.cc:34: #include "content/public/common/content_features.cc" On 2016/07/11 02:35:25, kinuko wrote: > content_features.h ? Sorry. Fixed.
Looking good, could we run this on dry CQ? https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/100001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:219: ResourceRequestInfoImpl* info = new ResourceRequestInfoImpl( On 2016/07/11 05:16:36, Adam Rice wrote: > On 2016/07/11 02:35:24, kinuko wrote: > > Note that ResourceRequestInfo::AllocateForTesting method has an explicit > comment > > like 'NOTE: Add more parameters if you need to initialize other fields', so I > > think we can try adding the filter thing to it too. (No other test code seems > to > > directly use this ctor) > > > > (As it requires more changes in other test code it can be done in a separate > CL > > too) > > ResourceRequestInfo::AllocateForTesting() only sets fields which are exposed by > ResourceRequestInfo. This doesn't include the ResourceContext field. This makes > sense from an encapsulation point of view. > > I thought about adding a ResourceRequestInfoImpl::AllocateForTesting() function, > but I concluded it would be better just to simplify the ResourceRequestInfoImpl > constructor. I see... then we probably need to live with this lengthy ctor. https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:21: #include "base/memory/shared_memory_handle.h" Do we need this? (Just checking, no need to add comments etc - here and below) https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:24: #include "base/process/process_handle.h" Do we need this? https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:26: #include "base/strings/string_number_conversions.h" Do we still need this? https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:63: "Content-Length: %zu\n", nit: "%" PRIuS from base/format_macros.h ? https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:251: finish_waiter_->Quit(); nit: Could we hit non-null case in the current code? Otherwise we can just remove the 'if' https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:275: ResourceMsg_DataReceived::Read(messages[2].get(), ¶ms); nit: we could also do something like template <typename MESSAGE> bool ReadMessageParam(IPC::Message* msg, typename MESSAGE::Param* params) { return MessageClass::ID == msg->type() && MESSAGE::Read(msg, params); } ASSERT_TRUE(ReadMessageParam<...>(messages[2].get(), ¶ms)); ...but just writing out like the current code feels fine / easier to follow to me.
The CQ bit was checked by ricea@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...)
The CQ bit was checked by ricea@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...
I have rebased so that the CQ dry run works. https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:21: #include "base/memory/shared_memory_handle.h" On 2016/07/11 06:43:38, kinuko wrote: > Do we need this? (Just checking, no need to add comments etc - here and below) No. ResourceMsg_SetDataBuffer::Param's destructor has to work without our help. Removed. https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:24: #include "base/process/process_handle.h" On 2016/07/11 06:43:39, kinuko wrote: > Do we need this? Removed. https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:26: #include "base/strings/string_number_conversions.h" On 2016/07/11 06:43:39, kinuko wrote: > Do we still need this? Removed. The switch to StringPrintf made it unnecessary. https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:63: "Content-Length: %zu\n", On 2016/07/11 06:43:39, kinuko wrote: > nit: "%" PRIuS from base/format_macros.h ? Done. https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:251: finish_waiter_->Quit(); On 2016/07/11 06:43:39, kinuko wrote: > nit: Could we hit non-null case in the current code? Otherwise we can just > remove the 'if' No. Tests can't start a request except by calling CreateStartAndWait(), and that sets finish_waiter_. I removed the 'if'. https://codereview.chromium.org/2092993002/diff/140001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:275: ResourceMsg_DataReceived::Read(messages[2].get(), ¶ms); On 2016/07/11 06:43:38, kinuko wrote: > nit: we could also do something like > > template <typename MESSAGE> > bool ReadMessageParam(IPC::Message* msg, typename MESSAGE::Param* params) { > return MessageClass::ID == msg->type() && MESSAGE::Read(msg, params); > } > > ASSERT_TRUE(ReadMessageParam<...>(messages[2].get(), ¶ms)); > > ...but just writing out like the current code feels fine / easier to follow to > me. Yes. The compiler might even be able to infer the template parameter. I think that's something that can be revisited when there are more tests.
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...)
The CQ bit was checked by ricea@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: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2092993002/diff/220001/content/browser/loader... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2092993002/diff/220001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:181: void CreateRequest(size_t response_data_size) { nit: CreateRequestWithResponseDataSize or something like that to make the parameter a bit clearer in the call site? https://codereview.chromium.org/2092993002/diff/220001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:189: filter_ = make_scoped_refptr( nit: probably we can remove this make_scoped_refptr https://codereview.chromium.org/2092993002/diff/220001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:228: void CreateStartAndWait(size_t response_data_size) { nit: the name is a bit hard to parse... (so many verbs) StartRequestAndWaitWithResponseDataSize or something like that?
The CQ bit was checked by ricea@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_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 ricea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ksakamoto@chromium.org, mkwst@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2092993002/#ps240001 (title: "Method renames")
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
Try jobs failed on following builders: linux_chromium_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 ricea@chromium.org
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
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 ricea@chromium.org
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ricea@chromium.org
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 #13 (id:240001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Browser process changes for Resource Timing sizes. This is part of the changes to add the size fields to the PerformanceResourceTiming API. See the design doc at https://docs.google.com/document/d/1ckL-rKLFRsdI4nn1golvQ6I1zRIvxgFkDXMrZb8Kd... These are the browser-side changes to pass the "encodedBodySize" field to the renderer. The field is named encoded_body_length in the //content code for consistency with the existing encoded_data_length field. For async resource fetches the value is passed one chunk at a time in the ResourceMsg_InlinedDataChunkReceived and ResourceMsg_DataReceived IPCs. For sync XHR it is passed in the ResourceResponseInfo struct. BUG=467945 ========== to ========== Browser process changes for Resource Timing sizes. This is part of the changes to add the size fields to the PerformanceResourceTiming API. See the design doc at https://docs.google.com/document/d/1ckL-rKLFRsdI4nn1golvQ6I1zRIvxgFkDXMrZb8Kd... These are the browser-side changes to pass the "encodedBodySize" field to the renderer. The field is named encoded_body_length in the //content code for consistency with the existing encoded_data_length field. For async resource fetches the value is passed one chunk at a time in the ResourceMsg_InlinedDataChunkReceived and ResourceMsg_DataReceived IPCs. For sync XHR it is passed in the ResourceResponseInfo struct. BUG=467945 Committed: https://crrev.com/e7b920b234f45614806cc4467e3e602720839e1e Cr-Commit-Position: refs/heads/master@{#405015} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/e7b920b234f45614806cc4467e3e602720839e1e Cr-Commit-Position: refs/heads/master@{#405015} |
