Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2905763002:
Deduplicating CanReadRequestBody code. (Closed)
|
Created:
3 years, 7 months ago by Łukasz Anforowicz Modified:
3 years, 6 months ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, Randy Smith (Not in Mondays), darin-cc_chromium.org, loading-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDeduplicating CanReadRequestBody code.
The same validation of ResourceRequestBody needs to be done from
- RenderFrameHostImpl::OnBeginNavigation (renderer-initiated navigations
via PlzNavigate)
- RenderFrameProxyHost::OnOpenURL (renderer-initiated navigations that
navigate a remote frame; this is used with and without PlzNavigate).
- ResourceDispatcherHostImpl::ShouldServiceRequest (before PlzNavigate
this applies to navigations; after PlzNavigate this will only be used
for XHRs)
BUG=726067
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2905763002
Cr-Commit-Position: refs/heads/master@{#475693}
Committed: https://chromium.googlesource.com/chromium/src/+/5e8bef0e04a637592c3443eb58a32091449d987a
Patch Set 1 #Patch Set 2 : Tweaking content/browser/loader/DEPS #
Total comments: 7
Patch Set 3 : Rebasing - now resource_request_body_browser_utils.* files are added in the "parent" CL @ https://c… #Patch Set 4 : Rebasing... #
Depends on Patchset: Messages
Total messages: 37 (25 generated)
Description was changed from ========== Deduplicating CanReadRequestBody code. BUG=726067 ========== to ========== Deduplicating CanReadRequestBody code. BUG=726067 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lukasza@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Deduplicating CanReadRequestBody code. BUG=726067 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Deduplicating CanReadRequestBody code. The same validation of ResourceRequestBody needs to be done from - RenderFrameHostImpl::OnBeginNavigation (renderer-initiated navigations via PlzNavigate) - RenderFrameProxyHost::OnOpenURL (renderer-initiated navigations that navigate a remote frame; this is used with and without PlzNavigate). - ResourceDispatcherHostImpl::ShouldServiceRequest (before PlzNavigate this applies to navigations; after PlzNavigate this will only be used for XHRs) BUG=726067 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
lukasza@chromium.org changed reviewers: + mmenke@chromium.org, nick@chromium.org
mmenke@ - could you PTAL at the question I had about content/browser/loader/DEPS (see below). nick@ - could you PTAL at the overall CL (and at the question about where to put the new functions - see below). Thanks! https://codereview.chromium.org/2905763002/diff/20001/content/browser/loader/... File content/browser/loader/DEPS (right): https://codereview.chromium.org/2905763002/diff/20001/content/browser/loader/... content/browser/loader/DEPS:157: "+content/browser/resource_request_body_browser_utils.h", I assume that after decoupling from the rest of //content, content/browser/loader will define an abstract LoaderDelegate interface that will be used to interact with things that should stay in the //content. I assume that ResourceDispatcherHostImpl::ShouldServiceRequest(...) will move into LoaderDelegate::ShouldServiceRequest(...) that will overriden/implemented in the content layer. Therefore I think it is okay to add the dependency above - this dependency will stay in the //content layer in the end (i.e. the dependency would be used from ContentLoaderDelegate::ShouldServiceRequest). Does what I said above make sense? How can we deduplicate the code while still supporting the efforts for decoupling the loader from the rest of //content? https://codereview.chromium.org/2905763002/diff/20001/content/browser/resourc... File content/browser/resource_request_body_browser_utils.h (right): https://codereview.chromium.org/2905763002/diff/20001/content/browser/resourc... content/browser/resource_request_body_browser_utils.h:6: #define CONTENT_BROWSER_RESOURCE_REQUEST_BODY_BROWSER_UTILS_H_ I don't see another good place for the new helper functions. The functions depend on storage::FileSystemContext, so maybe they should go into content/browser/fileapi/browser_file_system_helper.h (similarily to PrepareDropDataForChildProcess), but the ResourceRequestBodyImpl encompasses more than just the filesystem stuff, browser_file_system_helper.h doesn't seem right. I think the new function(s) would be a great instance method of ResourceRequestBodyImpl, but the problem is that the new function should only be used from the browser (not from the renderer) and I don't see how to enforce this when by necessity ResourceRequestBodyImpl is declared in content/*common*/resource_request_body_impl.h. https://codereview.chromium.org/2905763002/diff/20001/content/browser/resourc... content/browser/resource_request_body_browser_utils.h:23: const scoped_refptr<ResourceRequestBodyImpl>& body); This overload is used from ResourceDispatcherHostImpl::ShouldServiceRequest. https://codereview.chromium.org/2905763002/diff/20001/content/browser/resourc... content/browser/resource_request_body_browser_utils.h:27: const scoped_refptr<ResourceRequestBodyImpl>& body); This overload is used from RenderFrameHostImpl::OnBeginNavigation and RenderFrameProxyHost::OnOpenURL.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2905763002/diff/20001/content/browser/loader/... File content/browser/loader/DEPS (right): https://codereview.chromium.org/2905763002/diff/20001/content/browser/loader/... content/browser/loader/DEPS:157: "+content/browser/resource_request_body_browser_utils.h", On 2017/05/25 16:11:37, Łukasz A. wrote: > I assume that after decoupling from the rest of //content, > content/browser/loader will define an abstract LoaderDelegate interface that > will be used to interact with things that should stay in the //content. I > assume that ResourceDispatcherHostImpl::ShouldServiceRequest(...) will move into > LoaderDelegate::ShouldServiceRequest(...) that will overriden/implemented in the > content layer. Therefore I think it is okay to add the dependency above - this > dependency will stay in the //content layer in the end (i.e. the dependency > would be used from ContentLoaderDelegate::ShouldServiceRequest). > > Does what I said above make sense? > > How can we deduplicate the code while still supporting the efforts for > decoupling the loader from the rest of //content? For better or for worse, the plan is now to make a service from scratch, so it won't be using this code at all. The RDH-equivalent will be in a separate process, unclear how much, if any, Chrome code will be in that process.
https://codereview.chromium.org/2905763002/diff/20001/content/browser/loader/... File content/browser/loader/DEPS (right): https://codereview.chromium.org/2905763002/diff/20001/content/browser/loader/... content/browser/loader/DEPS:157: "+content/browser/resource_request_body_browser_utils.h", On 2017/05/25 16:31:08, mmenke wrote: > On 2017/05/25 16:11:37, Łukasz A. wrote: > > I assume that after decoupling from the rest of //content, > > content/browser/loader will define an abstract LoaderDelegate interface that > > will be used to interact with things that should stay in the //content. I > > assume that ResourceDispatcherHostImpl::ShouldServiceRequest(...) will move > into > > LoaderDelegate::ShouldServiceRequest(...) that will overriden/implemented in > the > > content layer. Therefore I think it is okay to add the dependency above - > this > > dependency will stay in the //content layer in the end (i.e. the dependency > > would be used from ContentLoaderDelegate::ShouldServiceRequest). > > > > Does what I said above make sense? > > > > How can we deduplicate the code while still supporting the efforts for > > decoupling the loader from the rest of //content? > > For better or for worse, the plan is now to make a service from scratch, so it > won't be using this code at all. > > The RDH-equivalent will be in a separate process, unclear how much, if any, > Chrome code will be in that process. Does that mean that the change to the DEPS file looks okay?
Yea, the DEPs change (And RDH change) LGTM. Didn't look at underlying logic to compare it, I'll defer to ncarter there. We'll need to get the servicification story for this together at some point, but don't think this change hurts that. https://codereview.chromium.org/2905763002/diff/20001/content/browser/resourc... File content/browser/resource_request_body_browser_utils.h (right): https://codereview.chromium.org/2905763002/diff/20001/content/browser/resourc... content/browser/resource_request_body_browser_utils.h:23: const scoped_refptr<ResourceRequestBodyImpl>& body); On 2017/05/25 16:11:37, Łukasz A. wrote: > This overload is used from ResourceDispatcherHostImpl::ShouldServiceRequest. Overloads are generally discouraged. Also it might be nice if there were some way to figure out at callsites that this is coming from another file (static method in a class that's never instantiated? Prefix?)
Oh, and that comment is completely optional, just think the current code could leave me, at least, initially confused while looking for that method in the RDHI files themselves.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lukasza@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...
FYI - I had to move introduction of resource_request_body_browser_utils.* into the parent CL (https://crrev.com/2908433003), so some of the discussion around the new function will out of necessity happen in the other CL.
On 2017/05/25 20:02:33, Łukasz A. wrote: > FYI - I had to move introduction of resource_request_body_browser_utils.* into > the parent CL (https://crrev.com/2908433003), so some of the discussion around > the new function will out of necessity happen in the other CL. That's fine, happy to defer to reviewers there, just wanted to raise the issue.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lukasza@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
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2905763002/#ps60001 (title: "Rebasing...")
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_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by lukasza@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496176392884810,
"parent_rev": "e437543b5a4c01773a489e26659a9b05b7d7c365", "commit_rev":
"5e8bef0e04a637592c3443eb58a32091449d987a"}
Message was sent while issue was closed.
Description was changed from ========== Deduplicating CanReadRequestBody code. The same validation of ResourceRequestBody needs to be done from - RenderFrameHostImpl::OnBeginNavigation (renderer-initiated navigations via PlzNavigate) - RenderFrameProxyHost::OnOpenURL (renderer-initiated navigations that navigate a remote frame; this is used with and without PlzNavigate). - ResourceDispatcherHostImpl::ShouldServiceRequest (before PlzNavigate this applies to navigations; after PlzNavigate this will only be used for XHRs) BUG=726067 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Deduplicating CanReadRequestBody code. The same validation of ResourceRequestBody needs to be done from - RenderFrameHostImpl::OnBeginNavigation (renderer-initiated navigations via PlzNavigate) - RenderFrameProxyHost::OnOpenURL (renderer-initiated navigations that navigate a remote frame; this is used with and without PlzNavigate). - ResourceDispatcherHostImpl::ShouldServiceRequest (before PlzNavigate this applies to navigations; after PlzNavigate this will only be used for XHRs) BUG=726067 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2905763002 Cr-Commit-Position: refs/heads/master@{#475693} Committed: https://chromium.googlesource.com/chromium/src/+/5e8bef0e04a637592c3443eb58a3... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/5e8bef0e04a637592c3443eb58a3... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
