Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2481093003:
Introduce ResourceRequesterInfo to abstract the requester of resource request (Closed)
|
Created:
4 years, 1 month ago by horo Modified:
4 years ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, serviceworker-reviews, creis+watch_chromium.org, shimazu+serviceworker_chromium.org, nasko+codewatch_chromium.org, jam, Randy Smith (Not in Mondays), nhiroki, kinuko+serviceworker, darin-cc_chromium.org, blink-worker-reviews_chromium.org, loading-reviews_chromium.org, horo+watch_chromium.org, kinuko+watch, tzik, clamy, asanka Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce ResourceRequesterInfo to abstract the requester of resource requests.
Currently there are three types of resource request.
- Normal resource request from renderer process.
- Resource request for browser side navigation. (PlzNavigate).
- Download request or Page Save request.
In ResourceRequestInfoImpl, these type are distinguished by process_type and
the existence of ResourceMessageFilter. But it is a bit confusing.
For example:
- AsyncRevalidationManager is checking only the existence of the filter to
check that "the renderer is gone" or "the request is for PlzNavigate".
- service_worker_context field is added for PlzNavigate support. And the field
is used only when the filter is not available in HandleServiceWorkerLink().
This CL introduces ResourceRequesterInfo to make it easy to distinguish those
requester types.
BUG=468227, 649558
TBR=michaeln@chromium.org
Committed: https://crrev.com/227286f7cd60cb0ed8756cd1efefb2d694c9734f
Cr-Commit-Position: refs/heads/master@{#434957}
Patch Set 1 #Patch Set 2 #
Total comments: 28
Patch Set 3 : incorporated yhirano & kinuko's comment #
Total comments: 15
Patch Set 4 : incorporated mmemke's comment #
Total comments: 6
Patch Set 5 : rebase #Patch Set 6 : incorporated kinuko's comment #Patch Set 7 : fix unittests #
Total comments: 18
Patch Set 8 : incorporated mmenke's comment #Patch Set 9 : fix URLLoaderFactoryImplTest.CancelFromRenderer #
Total comments: 12
Patch Set 10 : incorporated kinuko's comment #
Total comments: 17
Patch Set 11 : incorporated kinuko's comment #
Total comments: 39
Patch Set 12 : incorporated mmenke's comment #Patch Set 13 : make ResourceRequestInfoImpl::requester_info() non-const #
Total comments: 4
Patch Set 14 : incorporated mmenke's comment #
Total comments: 2
Patch Set 15 : s/esource/resource/ #
Total comments: 2
Patch Set 16 : incorporated yhirano's comment #Patch Set 17 : fix test failure #Patch Set 18 : fix URLLoaderFactoryImplTest/URLLoaderFactoryImplTest.GetFailedResponse2 #Messages
Total messages: 193 (156 generated)
The CQ bit was checked by horo@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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by horo@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 horo@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 horo@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by horo@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 checked by horo@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 checked by horo@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 horo@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 checked by horo@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...
Description was changed from ========== requester_info BUG= ========== to ========== [Not ready for review] Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for PlzNavigate. - Download request of Page Save request. (I'm planing to introduce a new type for service worker navigation request.) In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter for "the renderer is gone" of "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. ResourceRequesterInfo has accessors of ServiceWorkerContext. This will be used to fix crbug.com/468227. ==========
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
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
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 horo@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.
The CQ bit was checked by horo@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 checked by horo@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.
The CQ bit was checked by horo@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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by horo@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...
Description was changed from ========== [Not ready for review] Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for PlzNavigate. - Download request of Page Save request. (I'm planing to introduce a new type for service worker navigation request.) In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter for "the renderer is gone" of "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. ResourceRequesterInfo has accessors of ServiceWorkerContext. This will be used to fix crbug.com/468227. ========== to ========== [Not ready for review] Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for PlzNavigate. - Download request of Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. ResourceRequesterInfo has accessors of ServiceWorkerContext. This will be used to fix crbug.com/468227. ==========
Description was changed from ========== [Not ready for review] Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for PlzNavigate. - Download request of Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. ResourceRequesterInfo has accessors of ServiceWorkerContext. This will be used to fix crbug.com/468227. ========== to ========== [Not ready for review] Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request of Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. ResourceRequesterInfo has accessors of ServiceWorkerContext. This will be used to fix crbug.com/468227. ==========
Description was changed from ========== [Not ready for review] Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request of Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. ResourceRequesterInfo has accessors of ServiceWorkerContext. This will be used to fix crbug.com/468227. ========== to ========== [Not ready for review] Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request of Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. ==========
Patchset #5 (id:280001) has been deleted
Patchset #4 (id:260001) has been deleted
Patchset #3 (id:240001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #2 (id:300001) has been deleted
The CQ bit was checked by horo@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...
Description was changed from ========== [Not ready for review] Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request of Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. ========== to ========== Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request of Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. ==========
horo@chromium.org changed reviewers: + yhirano@chromium.org
yhirano@ Could you please review this?
Description was changed from ========== Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request of Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. ========== to ========== Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request of Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. BUG=468227,649558 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2481093003/diff/320001/content/browser/appcac... File content/browser/appcache/appcache_interceptor.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/appcac... content/browser/appcache/appcache_interceptor.cc:15: #include "content/browser/loader/resource_message_filter.h" Note needed? https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/async_resource_handler.cc:23: #include "content/browser/loader/resource_message_filter.h" Not needed. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/async_resource_handler.cc:218: reported_encoded_body_length_(0) { Adding DCHECK(GetRequestInfo()->requester_info().IsRenderer()); might be good. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (left): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1271: if ((process_type == PROCESS_TYPE_RENDERER) && is_navigation_stream_request Does this change mean this |process_type| check is completely redundant? Ditto for other places calling ResourceMessageFilter::process_type(). https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/sync_resource_handler.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/sync_resource_handler.cc:11: #include "content/browser/loader/resource_message_filter.h" Not needed. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/url_loader_factory_impl.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/url_loader_factory_impl.cc:8: #include "content/browser/loader/resource_message_filter.h" Not needed. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/url_loader_factory_impl.h (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/url_loader_factory_impl.h:15: class ResourceMessageFilter; Not needed. https://codereview.chromium.org/2481093003/diff/320001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:16: #include "content/browser/loader/resource_message_filter.h" Not needed.
kinuko@chromium.org changed reviewers: + kinuko@chromium.org, mmenke@chromium.org
Drive-by... I feel this could be a good cleanup but feels a bit rough. It might be worth writing a short doc about your proposal as multiple module owners in content could be interested in? I made some comments inline but I'd like to hear from other owners too https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:745: std::unique_ptr<ResourceRequesterInfo> requester_info_; RDH is browser process global object, owning ResourceRequesterInfo for each request feels wrong to me. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/resource_message_filter.h (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/resource_message_filter.h:119: scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_; I feel if we introduce something like ResourceRequesterInfo we might also want to get rid of these fields from RMF? Could we create a RequesterInfo when we create RMF and let it own the requester info (but interested in how other people think about this)? https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/resource_requester_info.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/resource_requester_info.cc:17: class ResourceRequesterInfoForRenderer : public ResourceRequesterInfo { Do we really need all these subclasses? https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/resource_requester_info.h (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/resource_requester_info.h:33: class CONTENT_EXPORT ResourceRequesterInfo { Currently this one and RMF have somewhat overlapping fields / methods, which makes the purpose of this class a little unclear. Could we more clearly split the role between them? https://codereview.chromium.org/2481093003/diff/320001/content/browser/servic... File content/browser/service_worker/link_header_support.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/servic... content/browser/service_worker/link_header_support.cc:58: : request_info->requester_info().service_worker_context(); Isn't this doing a little different thing from the original code? (Original code was trying to use filter's service_worker_context before checking sw_context_for_testing)
The CQ bit was checked by horo@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/2481093003/diff/320001/content/browser/appcac... File content/browser/appcache/appcache_interceptor.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/appcac... content/browser/appcache/appcache_interceptor.cc:15: #include "content/browser/loader/resource_message_filter.h" On 2016/11/10 06:42:29, yhirano wrote: > Note needed? Done. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/async_resource_handler.cc:23: #include "content/browser/loader/resource_message_filter.h" On 2016/11/10 06:42:29, yhirano wrote: > Not needed. Done. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/async_resource_handler.cc:218: reported_encoded_body_length_(0) { On 2016/11/10 06:42:29, yhirano wrote: > Adding DCHECK(GetRequestInfo()->requester_info().IsRenderer()); might be good. Done. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (left): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1271: if ((process_type == PROCESS_TYPE_RENDERER) && is_navigation_stream_request On 2016/11/10 06:42:29, yhirano wrote: > Does this change mean this |process_type| check is completely redundant? Ditto > for other places calling ResourceMessageFilter::process_type(). These checks were added by this change https://codereview.chromium.org/2283473002 to fix URLLoaderFactoryImplTest with PlzNavigate enabled. The test failed with PlzNavigate because: - The test fetches frame type requests with mojom::URLLoader. - But frame type requests which URL is not blob scheme is blocked when PlzNavigate is enabled. So the cl (2283473002) introduced this check and changed the process type in the test to PROCESS_TYPE_UNKNOWN. We can remove these checks by just changing URLLoaderFactoryImplTest not to fetch frame type requests. And I think this is better approach. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:745: std::unique_ptr<ResourceRequesterInfo> requester_info_; On 2016/11/10 08:17:57, kinuko wrote: > RDH is browser process global object, owning ResourceRequesterInfo for each > request feels wrong to me. Done. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/resource_message_filter.h (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/resource_message_filter.h:119: scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_; On 2016/11/10 08:17:57, kinuko wrote: > I feel if we introduce something like ResourceRequesterInfo we might also want > to get rid of these fields from RMF? Could we create a RequesterInfo when we > create RMF and let it own the requester info (but interested in how other people > think about this)? Humm.. I don't have a good idea. I think this "ResourceRequesterInfo of renderer has a pointer to RMF, and call RMF's methods to get those contexts" is not a bad idea. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/resource_requester_info.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/resource_requester_info.cc:17: class ResourceRequesterInfoForRenderer : public ResourceRequesterInfo { On 2016/11/10 08:17:57, kinuko wrote: > Do we really need all these subclasses? I think making subclasses for each purpose is easier to understand than adding "if(RENDERER){} else if(BROWSER_SIDE_NAVIGATION) {} else {}" for each methods. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/resource_requester_info.h (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/resource_requester_info.h:33: class CONTENT_EXPORT ResourceRequesterInfo { On 2016/11/10 08:17:57, kinuko wrote: > Currently this one and RMF have somewhat overlapping fields / methods, which > makes the purpose of this class a little unclear. Could we more clearly split > the role between them? ResourceRequesterInfoForRenderer can get the contexts from RMF. But ForBrowserSideNavigation and ForDownloadOrPageSave can't. ResourceRequesterInfoForBrowserSideNavigation has scoped_refptr<ServiceWorkerContextWrapper> which was acquired from ServiceWorkerNavigationHandle on UI thread. I think to support ServiceWorker for download and SavePage (https://crbug.com/468227), I have to find a way to pass ServiceWorkerContext to ResourceRequesterInfoForDownloadOrPageSave. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/sync_resource_handler.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/sync_resource_handler.cc:11: #include "content/browser/loader/resource_message_filter.h" On 2016/11/10 06:42:29, yhirano wrote: > Not needed. Done. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/url_loader_factory_impl.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/url_loader_factory_impl.cc:8: #include "content/browser/loader/resource_message_filter.h" On 2016/11/10 06:42:29, yhirano wrote: > Not needed. Done. https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/url_loader_factory_impl.h (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/url_loader_factory_impl.h:15: class ResourceMessageFilter; On 2016/11/10 06:42:29, yhirano wrote: > Not needed. Done. https://codereview.chromium.org/2481093003/diff/320001/content/browser/servic... File content/browser/service_worker/link_header_support.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/servic... content/browser/service_worker/link_header_support.cc:58: : request_info->requester_info().service_worker_context(); On 2016/11/10 08:17:57, kinuko wrote: > Isn't this doing a little different thing from the original code? (Original > code was trying to use filter's service_worker_context before checking > sw_context_for_testing) Yes, there is a difference. But there is no problem. service_worker_context_for_testing is used only by LinkHeaderServiceWorkerTest to pass the SWContext. https://codereview.chromium.org/2481093003/diff/320001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:16: #include "content/browser/loader/resource_message_filter.h" On 2016/11/10 06:42:30, yhirano wrote: > Not needed. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Preliminary comments, sorry for the delay. https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1064: DCHECK(requester_info->IsRenderer()); Is ResourceDispatcher limited to just be in renderer processes? No plugin processes, etc? That's news to me. https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... File content/browser/loader/resource_requester_info.cc (right): https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_requester_info.cc:181: DCHECK(filter); If we're DCHECKing, should we be taking a ResourceMessageFilter* instead? Seems silly to call AsWeakPtr at all callsites. Or is the plan that AsWeakPtr will eventually become a private method, when RDH is no longer creating any of these? https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... File content/browser/loader/resource_requester_info.h (right): https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_requester_info.h:32: // - Download request of page save request. A downloads also use this path, no? https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_requester_info.h:37: static std::unique_ptr<ResourceRequesterInfo> CreateForRenderer( include <memory> https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_requester_info.h:45: // Creates a ResourceRequesterInfo for browser side navigation request which Need an article here, or make it plural. Same for "browser process". https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_requester_info.h:63: virtual int child_id() const = 0; virtual methods shouldn't use this naming scheme. https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_requester_info.h:74: net::URLRequestContext** request_context) const = 0; This method and those below it all crash if the filter has been destroyed. child_id() does not. Some documentation of behavior here?
The CQ bit was checked by horo@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...)
Patchset #4 (id:360001) has been deleted
The CQ bit was checked by horo@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_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
(response only, no new review comments) https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/resource_message_filter.h (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/resource_message_filter.h:119: scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_; On 2016/11/10 14:46:33, horo wrote: > On 2016/11/10 08:17:57, kinuko wrote: > > I feel if we introduce something like ResourceRequesterInfo we might also want > > to get rid of these fields from RMF? Could we create a RequesterInfo when we > > create RMF and let it own the requester info (but interested in how other > people > > think about this)? > > Humm.. I don't have a good idea. > I think this "ResourceRequesterInfo of renderer has a pointer to RMF, and call > RMF's methods to get those contexts" is not a bad idea. RMF's role to hold storage contexts are basically for RDH to access them, but now the role seems to be completely replaced by ResourceRequesterInfo, whose name sounded more like a place to hold requester context and seems to be deprecating the role from RMF for renderer cases. Looking the (new) patch again I see RRI is going to be more short-lived / will have different life span than RMF? I will take another look... https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... File content/browser/loader/resource_requester_info.cc (right): https://codereview.chromium.org/2481093003/diff/320001/content/browser/loader... content/browser/loader/resource_requester_info.cc:17: class ResourceRequesterInfoForRenderer : public ResourceRequesterInfo { On 2016/11/10 14:46:33, horo wrote: > On 2016/11/10 08:17:57, kinuko wrote: > > Do we really need all these subclasses? > > I think making subclasses for each purpose is easier to understand than adding > "if(RENDERER){} else if(BROWSER_SIDE_NAVIGATION) {} else {}" for each methods. I imagined this class could be a mere collection of necessary data fields that can be initialized differently for each type. (Probably the class name 'info' made me expect that) https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1064: DCHECK(requester_info->IsRenderer()); On 2016/11/11 21:19:59, mmenke wrote: > Is ResourceDispatcher limited to just be in renderer processes? No plugin > processes, etc? That's news to me. After NPAPI support gets removed it looks the only process type that creates RMF (and use RDH) seems to be renderer processes. (Which probably means we could clean up a little more code)
The CQ bit was checked by horo@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/2481093003/diff/340001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1064: DCHECK(requester_info->IsRenderer()); On 2016/11/11 21:19:59, mmenke wrote: > Is ResourceDispatcher limited to just be in renderer processes? No plugin > processes, etc? That's news to me. There is no PROCESS_TYPE_PLUGIN after NPAPI was removed. https://codereview.chromium.org/1862513003 PROCESS_TYPE_WORKER was removed when SharedWorker was moved to the rendere process. https://codereview.chromium.org/411283002 https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... File content/browser/loader/resource_requester_info.cc (right): https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_requester_info.cc:181: DCHECK(filter); On 2016/11/11 21:19:59, mmenke wrote: > If we're DCHECKing, should we be taking a ResourceMessageFilter* instead? Seems > silly to call AsWeakPtr at all callsites. Or is the plan that AsWeakPtr will > eventually become a private method, when RDH is no longer creating any of these? Taking a ResourceMessageFilter* sounds good. Done. https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... File content/browser/loader/resource_requester_info.h (right): https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_requester_info.h:32: // - Download request of page save request. On 2016/11/11 21:19:59, mmenke wrote: > A downloads also use this path, no? It was a typo. Fixed. https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_requester_info.h:37: static std::unique_ptr<ResourceRequesterInfo> CreateForRenderer( On 2016/11/11 21:19:59, mmenke wrote: > include <memory> Done. https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_requester_info.h:45: // Creates a ResourceRequesterInfo for browser side navigation request which On 2016/11/11 21:19:59, mmenke wrote: > Need an article here, or make it plural. Same for "browser process". Done. https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_requester_info.h:63: virtual int child_id() const = 0; On 2016/11/11 21:19:59, mmenke wrote: > virtual methods shouldn't use this naming scheme. Done. https://codereview.chromium.org/2481093003/diff/340001/content/browser/loader... content/browser/loader/resource_requester_info.h:74: net::URLRequestContext** request_context) const = 0; On 2016/11/11 21:19:59, mmenke wrote: > This method and those below it all crash if the filter has been destroyed. > child_id() does not. Some documentation of behavior here? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2481093003/diff/380001/content/browser/loader... File content/browser/loader/resource_message_filter.h (right): https://codereview.chromium.org/2481093003/diff/380001/content/browser/loader... content/browser/loader/resource_message_filter.h:119: scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_; I still feel having these fields is redundant and a bit confusing now that we have requester info. In SW-preload case we hold requester info in URLLoaderFactoryImpl and clone the info when it handles each resource request. Wondering if doing similar for other cases make sense, i.e. creating RRI when we create RMF (RRI doesn't really seem to need to be per resource request) and pass it or clone it when we handle each request? https://codereview.chromium.org/2481093003/diff/380001/content/browser/loader... File content/browser/loader/resource_requester_info.cc (right): https://codereview.chromium.org/2481093003/diff/380001/content/browser/loader... content/browser/loader/resource_requester_info.cc:17: class ResourceRequesterInfoForRenderer : public ResourceRequesterInfo { (Again) I think we could also make it a simple data-type class that just initializes all necessary member fields when we create an instance without making subclasses. And for renderer cases we could just remove duplicated member fields from RMF... would that work / make some sense to you or not? https://codereview.chromium.org/2481093003/diff/380001/content/browser/loader... content/browser/loader/resource_requester_info.cc:201: return base::MakeUnique<ResourceRequesterInfoForDownloadOrPageSave>(child_id); Will we need to access service worker context for download cases? How do you plan to get that? Will we retrieve it via RPH with UI thread hop without using service_worker_context() field of this class?
The CQ bit was checked by horo@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/2481093003/diff/380001/content/browser/loader... File content/browser/loader/resource_message_filter.h (right): https://codereview.chromium.org/2481093003/diff/380001/content/browser/loader... content/browser/loader/resource_message_filter.h:119: scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_; On 2016/11/14 16:29:41, kinuko wrote: > I still feel having these fields is redundant and a bit confusing now that we > have requester info. > > In SW-preload case we hold requester info in URLLoaderFactoryImpl and clone the > info when it handles each resource request. Wondering if doing similar for > other cases make sense, i.e. creating RRI when we create RMF (RRI doesn't really > seem to need to be per resource request) and pass it or clone it when we handle > each request? Done. https://codereview.chromium.org/2481093003/diff/380001/content/browser/loader... File content/browser/loader/resource_requester_info.cc (right): https://codereview.chromium.org/2481093003/diff/380001/content/browser/loader... content/browser/loader/resource_requester_info.cc:17: class ResourceRequesterInfoForRenderer : public ResourceRequesterInfo { On 2016/11/14 16:29:41, kinuko wrote: > (Again) I think we could also make it a simple data-type class that just > initializes all necessary member fields when we create an instance without > making subclasses. And for renderer cases we could just remove duplicated > member fields from RMF... would that work / make some sense to you or not? Done. https://codereview.chromium.org/2481093003/diff/380001/content/browser/loader... content/browser/loader/resource_requester_info.cc:201: return base::MakeUnique<ResourceRequesterInfoForDownloadOrPageSave>(child_id); On 2016/11/14 16:29:41, kinuko wrote: > Will we need to access service worker context for download cases? How do you > plan to get that? Will we retrieve it via RPH with UI thread hop without using > service_worker_context() field of this class? I don't fully understand the download behavior yet. But DownloadManagerImpl lives on UI thread. So we can get the service worker context in DownloadManagerImpl without adding another thread hop. Created a POC patch: https://codereview.chromium.org/2498193003/#ps60001 https://codereview.chromium.org/2498193003/diff/60001/content/browser/downloa...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
rdsmith@chromium.org changed reviewers: + rdsmith@chromium.org
[+Asanka for DownloadManagerImpl involvement mentioned in last comment.]
The CQ bit was checked by horo@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...
Beyond working towards fixing the bug, I think this is a good CL - love that it gets rid of filter_. May do a second pass today, still digging into the CL. https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/async_resource_handler.cc:220: DCHECK(GetRequestInfo()->requester_info().IsRenderer()); include base/logging.h https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/async_resource_handler.cc:265: DCHECK(GetRequestInfo()->requester_info().IsRenderer()); Is this needed? This is constant for the lifetime of a request, right? Same for all of these except the one in the constructor. https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1360: ContinuePendingBeginRequest(requester_info->clone(), request_id, request_data, Should we make requester_info refcounted, and pass it around as a scoped_refptr<const RequestorInfo>? In general, I'm against refcounting, but given that this is an effectively constant class, seems reasonable. Doesn't save us a huGe amount, though it does mean we don't need clone() calls. https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... File content/browser/loader/resource_message_filter.cc (right): https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_message_filter.cc:46: requester_info_->set_filter(GetWeakPtr()); Comment that this can't be done on construction because the filter is created on the UI thread? At least I assume that's why this isn't done on construction. Maybe add a DCHECK_CURRENTLY_ON(BrowserThread::UI) to the constructor, just for documentation purposes? https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... File content/browser/loader/resource_request_info_impl.h (right): https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:112: return *(requester_info_.get()); nit: Is the get actually needed? https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... File content/browser/loader/resource_requester_info.cc (right): https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_requester_info.cc:49: DCHECK(type_ == RequesterType::RENDERER); DCHECK_EQ https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_requester_info.cc:49: DCHECK(type_ == RequesterType::RENDERER); include base/logging.h https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... File content/browser/loader/resource_requester_info.h (right): https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_requester_info.h:86: // may return null if the process eixted. This method is available only for exited https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_requester_info.h:144: GetContextsCallback get_contexts_callback_; I think all of these be const, except filter? (That is, const scoped_refptr, not scoped_refptr<const...>)
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_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #8 (id:460001) has been deleted
The CQ bit was checked by horo@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/2481093003/diff/440001/content/browser/loader... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/async_resource_handler.cc:220: DCHECK(GetRequestInfo()->requester_info().IsRenderer()); On 2016/11/17 16:27:19, mmenke wrote: > include base/logging.h Already included. https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/async_resource_handler.cc:265: DCHECK(GetRequestInfo()->requester_info().IsRenderer()); On 2016/11/17 16:27:19, mmenke wrote: > Is this needed? This is constant for the lifetime of a request, right? Same > for all of these except the one in the constructor. Done. https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1360: ContinuePendingBeginRequest(requester_info->clone(), request_id, request_data, On 2016/11/17 16:27:19, mmenke wrote: > Should we make requester_info refcounted, and pass it around as a > scoped_refptr<const RequestorInfo>? In general, I'm against refcounting, but > given that this is an effectively constant class, seems reasonable. Doesn't > save us a huGe amount, though it does mean we don't need clone() calls. Done. https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... File content/browser/loader/resource_message_filter.cc (right): https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_message_filter.cc:46: requester_info_->set_filter(GetWeakPtr()); On 2016/11/17 16:27:20, mmenke wrote: > Comment that this can't be done on construction because the filter is created on > the UI thread? At least I assume that's why this isn't done on construction. > > Maybe add a DCHECK_CURRENTLY_ON(BrowserThread::UI) to the constructor, just for > documentation purposes? Done. Added comment. https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... File content/browser/loader/resource_request_info_impl.h (right): https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:112: return *(requester_info_.get()); On 2016/11/17 16:27:20, mmenke wrote: > nit: Is the get actually needed? Done. https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... File content/browser/loader/resource_requester_info.cc (right): https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_requester_info.cc:49: DCHECK(type_ == RequesterType::RENDERER); On 2016/11/17 16:27:20, mmenke wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_requester_info.cc:49: DCHECK(type_ == RequesterType::RENDERER); On 2016/11/17 16:27:20, mmenke wrote: > include base/logging.h Done. https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... File content/browser/loader/resource_requester_info.h (right): https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_requester_info.h:86: // may return null if the process eixted. This method is available only for On 2016/11/17 16:27:20, mmenke wrote: > exited Done. https://codereview.chromium.org/2481093003/diff/440001/content/browser/loader... content/browser/loader/resource_requester_info.h:144: GetContextsCallback get_contexts_callback_; On 2016/11/17 16:27:20, mmenke wrote: > I think all of these be const, except filter? (That is, const scoped_refptr, > not scoped_refptr<const...>) Done.
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 horo@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_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Thanks, I like the new code much better now! Almost looking good for the part I care about, mod some more minor comments https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:102: OnFilterAdded(nullptr); Needing to OnFilterAdded even with nullptr in unittests is not obvious, could we have a comment about that in resource_message_filter.h? (Or having a static method like CreateForTesting which initializes all storage contexts with nullptr and call OnFilterAdded might be helpful?) https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:231: BrowserMessageFilter* filter, nit: this could probably just take it as IPC::Sender* (and rename filter -> sender), which would make the intention clearer https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... File content/browser/loader/resource_message_filter.h (right): https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... content/browser/loader/resource_message_filter.h:55: // process. Now that we're making it very explicit that this class is only for renderer in the code can we also update the comment? https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... File content/browser/loader/url_loader_factory_impl.cc (right): https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... content/browser/loader/url_loader_factory_impl.cc:37: const ResourceRequesterInfo* requester_info) nit: Since this actually takes a ref it'd be better to give it as scoped_refptr https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... content/browser/loader/url_loader_factory_impl.cc:99: const ResourceRequesterInfo* requester_info, ditto, passing it as scoped_refptr would make it clearer that it takes the ref https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... File content/browser/loader/url_loader_factory_impl.h (right): https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... content/browser/loader/url_loader_factory_impl.h:18: // a mojom::URLLoader. nit: not really related to this CL but I felt it'd be helpful to have a comment to note that this is instantiated for SW preload or test cases only
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Patchset #10 (id:520001) has been deleted
The CQ bit was checked by horo@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...
Patchset #10 (id:540001) has been deleted
The CQ bit was checked by horo@chromium.org to run a CQ dry run
https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:102: OnFilterAdded(nullptr); On 2016/11/18 01:29:21, kinuko wrote: > Needing to OnFilterAdded even with nullptr in unittests is not obvious, could we > have a comment about that in resource_message_filter.h? (Or having a static > method like CreateForTesting which initializes all storage contexts with nullptr > and call OnFilterAdded might be helpful?) Done. https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:231: BrowserMessageFilter* filter, On 2016/11/18 01:29:21, kinuko wrote: > nit: this could probably just take it as IPC::Sender* (and rename filter -> > sender), which would make the intention clearer Done. https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... File content/browser/loader/resource_message_filter.h (right): https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... content/browser/loader/resource_message_filter.h:55: // process. On 2016/11/18 01:29:21, kinuko wrote: > Now that we're making it very explicit that this class is only for renderer in > the code can we also update the comment? Done. https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... File content/browser/loader/url_loader_factory_impl.cc (right): https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... content/browser/loader/url_loader_factory_impl.cc:37: const ResourceRequesterInfo* requester_info) On 2016/11/18 01:29:21, kinuko wrote: > nit: Since this actually takes a ref it'd be better to give it as scoped_refptr Done. https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... content/browser/loader/url_loader_factory_impl.cc:99: const ResourceRequesterInfo* requester_info, On 2016/11/18 01:29:21, kinuko wrote: > ditto, passing it as scoped_refptr would make it clearer that it takes the ref Done. https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... File content/browser/loader/url_loader_factory_impl.h (right): https://codereview.chromium.org/2481093003/diff/500001/content/browser/loader... content/browser/loader/url_loader_factory_impl.h:18: // a mojom::URLLoader. On 2016/11/18 01:29:21, kinuko wrote: > nit: not really related to this CL but I felt it'd be helpful to have a comment > to note that this is instantiated for SW preload or test cases only Done.
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...)
The CQ bit was checked by horo@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...
Description was changed from ========== Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request of Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. BUG=468227,649558 ========== to ========== Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request or Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. BUG=468227,649558 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, lgtm for the part I care about % some more nits https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:231: IPC::Sender* filter, nit: I'd also rename |filter| to |sender| https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:529: void OnRequestResource(const ResourceRequesterInfo* requester_info, Wondering if many of these methods should also take scoped_refptr (and std::move along) as some of them are eventually taking the ref for async handling. (I'd like to defer the final decision to module owner / mmenke though) https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... File content/browser/loader/resource_message_filter.cc (right): https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_message_filter.cc:63: bool ResourceMessageFilter::OnMessageReceived(const IPC::Message& message) { nit: have some DCHECK to check if requester_info_->filter() is already initialized? https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... File content/browser/loader/resource_message_filter.h (right): https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_message_filter.h:52: // |service_worker_context| may be NULL in unittests. nit: NULL -> nullptr would be probably more useful to note that InitializeOnIOThread needs to be manually called for unittests here (rather than at InitializeOnIOThread) https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_message_filter.h:87: // after creating the ResourceMessageFilter. nit: maybe just comment about what this does here (e.g. "Initializes the weak pointer of this filter in requester_info_.") and move this comment at the ctor https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... File content/browser/loader/resource_requester_info.h (right): https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_requester_info.h:32: // This class represents the type of resource request. resource request -> resource requester? Would be better to update comments below too to reflect that it's more about per-requester rather than per-request https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_requester_info.h:36: // - Download request or page save request. - Requesters that request resource from renderer processes, - Requesters that request resource within the browser process for browser side navigation (i.e. PlzNavigate), and - Requesters that request resource for download or page save. ? https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_requester_info.h:45: // Creates a ResourceRequesterInfo for renderer process initiated request. ditto for per-request vs per-requester (here and below) https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_requester_info.h:85: ResourceMessageFilter* filter() const { return filter_.get(); } basically we shouldn't return non-const pointers from const methods (though some of the existing storage context getters below from RMF were not like that but it'd be nice if we fix it for the parts that don't require heavy lifting)
The CQ bit was checked by horo@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...
Patchset #11 (id:580001) has been deleted
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by horo@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/2481093003/diff/560001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:231: IPC::Sender* filter, On 2016/11/21 13:36:25, kinuko wrote: > nit: I'd also rename |filter| to |sender| Done. https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... File content/browser/loader/resource_message_filter.cc (right): https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_message_filter.cc:63: bool ResourceMessageFilter::OnMessageReceived(const IPC::Message& message) { On 2016/11/21 13:36:25, kinuko wrote: > nit: have some DCHECK to check if requester_info_->filter() is already > initialized? Done. https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... File content/browser/loader/resource_message_filter.h (right): https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_message_filter.h:52: // |service_worker_context| may be NULL in unittests. On 2016/11/21 13:36:25, kinuko wrote: > nit: NULL -> nullptr > > would be probably more useful to note that InitializeOnIOThread needs to be > manually called for unittests here (rather than at InitializeOnIOThread) Done. https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_message_filter.h:87: // after creating the ResourceMessageFilter. On 2016/11/21 13:36:26, kinuko wrote: > nit: maybe just comment about what this does here (e.g. "Initializes the weak > pointer of this filter in requester_info_.") and move this comment at the ctor Done. https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... File content/browser/loader/resource_requester_info.h (right): https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_requester_info.h:32: // This class represents the type of resource request. On 2016/11/21 13:36:26, kinuko wrote: > resource request -> resource requester? > > Would be better to update comments below too to reflect that it's more about > per-requester rather than per-request Done. https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_requester_info.h:36: // - Download request or page save request. On 2016/11/21 13:36:26, kinuko wrote: > - Requesters that request resource from renderer processes, > - Requesters that request resource within the browser process for browser side > navigation (i.e. PlzNavigate), and > - Requesters that request resource for download or page save. > > ? Done. https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_requester_info.h:45: // Creates a ResourceRequesterInfo for renderer process initiated request. On 2016/11/21 13:36:26, kinuko wrote: > ditto for per-request vs per-requester (here and below) Done. https://codereview.chromium.org/2481093003/diff/560001/content/browser/loader... content/browser/loader/resource_requester_info.h:85: ResourceMessageFilter* filter() const { return filter_.get(); } On 2016/11/21 13:36:26, kinuko wrote: > basically we shouldn't return non-const pointers from const methods (though some > of the existing storage context getters below from RMF were not like that but > it'd be nice if we fix it for the parts that don't require heavy lifting) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just a bunch of nits. Want to do a careful once over of RDH, but otherwise, happy with it. Sorry for the delays. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/async_resource_handler.cc:335: const ResourceRequestInfoImpl* info = GetRequestInfo(); nit: Move this down to just above the line where it's first used? Doesn't make any perf difference, since the filter basically always exists anyways, but google style is to declare variables just above first use. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/async_resource_handler.cc:484: const ResourceRequestInfoImpl* info = GetRequestInfo(); Move is to just above first use. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:137: } // namespace Any reason for moving this? https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:60: if (!info->requester_info().IsRenderer() || First check should actually be a DCHECK - the bool should never be set for renderer requests (Since they don't set the load flag LOAD_SUPPORT_ASYNC_REVALIDATION) https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/async_revalidation_manager_unittest.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:266: } // namespace Why this change? https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1133: const int child_id = requester_info->child_id(); nit: --const (const just tends not to be used for stack variables, unless it's needed for const references or the like) https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/resource_message_filter.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.cc:37: DCHECK_CURRENTLY_ON(BrowserThread::UI); include browser_thread.h https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.cc:66: DCHECK_EQ(this, requester_info_->filter()); Include base/logging.h https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.cc:108: // weak ptr of |requester_info_| now. nit: "weak ptr" -> WeakPtr? (x2). Or "weak pointer" https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/resource_message_filter.h (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.h:54: // OnFilterAdded() is not called. nit: "is not called" -> "would not otherwise be called" https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.h:83: friend class URLLoaderFactoryImplTest; This is in the wrong section, but see below comment about making a ForTesting version of methods it needs and making them public. Makes the API much clearer. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.h:95: friend class ResourceDispatcherHostTest; Instead of friending these, can we rename requester_info() to requester_info_for_testing() / for_test(), and make it public? Prefer that to giving tests access to the guts of this class. Alternatively, could modify the tests to go through the ResourceMessageFilter's OnMessageReceived method, if that works (Not sure if switching "just works", or if more work would be needed to hook things up) https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.h:100: scoped_refptr<ResourceRequesterInfo> requester_info_; include ref_counted https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/resource_request_info_impl.h (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:15: #include "base/memory/weak_ptr.h" No longer needed. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:28: class ResourceMessageFilter; No longer needed https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:111: ResourceRequesterInfo& requester_info() const { return *requester_info_; } const methods returning non-const references seems a little weird. I think this can just return a const reference? https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:200: scoped_refptr<ResourceRequesterInfo> requester_info_; Can this be scoped_refptr<const ResourceRequesterInfo>? https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/resource_requester_info.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_requester_info.cc:38: DCHECK_CURRENTLY_ON(BrowserThread::IO); include browser_thread https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/url_loader_factory_impl_unittest.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/url_loader_factory_impl_unittest.cc:153: request.request_initiator = url::Origin(); include url/origin.h
The CQ bit was checked by horo@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/2481093003/diff/600001/content/browser/loader... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/async_resource_handler.cc:335: const ResourceRequestInfoImpl* info = GetRequestInfo(); On 2016/11/21 19:51:55, mmenke wrote: > nit: Move this down to just above the line where it's first used? Doesn't make > any perf difference, since the filter basically always exists anyways, but > google style is to declare variables just above first use. Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/async_resource_handler.cc:484: const ResourceRequestInfoImpl* info = GetRequestInfo(); On 2016/11/21 19:51:55, mmenke wrote: > Move is to just above first use. Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/async_resource_handler_unittest.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/async_resource_handler_unittest.cc:137: } // namespace On 2016/11/21 19:51:55, mmenke wrote: > Any reason for moving this? Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/async_revalidation_manager.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/async_revalidation_manager.cc:60: if (!info->requester_info().IsRenderer() || On 2016/11/21 19:51:55, mmenke wrote: > First check should actually be a DCHECK - the bool should never be set for > renderer requests (Since they don't set the load flag > LOAD_SUPPORT_ASYNC_REVALIDATION) Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/async_revalidation_manager_unittest.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/async_revalidation_manager_unittest.cc:266: } // namespace On 2016/11/21 19:51:55, mmenke wrote: > Why this change? Done. reverted. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1133: const int child_id = requester_info->child_id(); On 2016/11/21 19:51:55, mmenke wrote: > nit: --const (const just tends not to be used for stack variables, unless it's > needed for const references or the like) Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/resource_message_filter.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.cc:37: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2016/11/21 19:51:55, mmenke wrote: > include browser_thread.h Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.cc:66: DCHECK_EQ(this, requester_info_->filter()); On 2016/11/21 19:51:55, mmenke wrote: > Include base/logging.h Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.cc:108: // weak ptr of |requester_info_| now. On 2016/11/21 19:51:55, mmenke wrote: > nit: "weak ptr" -> WeakPtr? (x2). Or "weak pointer" Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/resource_message_filter.h (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.h:54: // OnFilterAdded() is not called. On 2016/11/21 19:51:55, mmenke wrote: > nit: "is not called" -> "would not otherwise be called" Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.h:83: friend class URLLoaderFactoryImplTest; On 2016/11/21 19:51:55, mmenke wrote: > This is in the wrong section, but see below comment about making a ForTesting > version of methods it needs and making them public. Makes the API much clearer. Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.h:95: friend class ResourceDispatcherHostTest; On 2016/11/21 19:51:55, mmenke wrote: > Instead of friending these, can we rename requester_info() to > requester_info_for_testing() / for_test(), and make it public? Prefer that to > giving tests access to the guts of this class. Alternatively, could modify the > tests to go through the ResourceMessageFilter's OnMessageReceived method, if > that works (Not sure if switching "just works", or if more work would be needed > to hook things up) Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_message_filter.h:100: scoped_refptr<ResourceRequesterInfo> requester_info_; On 2016/11/21 19:51:55, mmenke wrote: > include ref_counted Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/resource_request_info_impl.h (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:15: #include "base/memory/weak_ptr.h" On 2016/11/21 19:51:55, mmenke wrote: > No longer needed. Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:28: class ResourceMessageFilter; On 2016/11/21 19:51:55, mmenke wrote: > No longer needed Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:111: ResourceRequesterInfo& requester_info() const { return *requester_info_; } On 2016/11/21 19:51:55, mmenke wrote: > const methods returning non-const references seems a little weird. I think this > can just return a const reference? Reflecting on kinuko's opinion, I changed ResourceRequesterInfo::filter() to non-const. So this method must return non-const. https://codereview.chromium.org/2481093003/diff2/560001:600001/content/browse... > basically we shouldn't return non-const pointers from const methods (though some > of the existing storage context getters below from RMF were not like that but > it'd be nice if we fix it for the parts that don't require heavy lifting) https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:200: scoped_refptr<ResourceRequesterInfo> requester_info_; On 2016/11/21 19:51:55, mmenke wrote: > Can this be scoped_refptr<const ResourceRequesterInfo>? Reflecting on kinuko's opinion, I changed ResourceRequesterInfo::filter() to non-const. So this must be non-const. https://codereview.chromium.org/2481093003/diff2/560001:600001/content/browse... > basically we shouldn't return non-const pointers from const methods (though some > of the existing storage context getters below from RMF were not like that but > it'd be nice if we fix it for the parts that don't require heavy lifting) https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/resource_requester_info.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_requester_info.cc:38: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2016/11/21 19:51:56, mmenke wrote: > include browser_thread Done. https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/url_loader_factory_impl_unittest.cc (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/url_loader_factory_impl_unittest.cc:153: request.request_initiator = url::Origin(); On 2016/11/21 19:51:56, mmenke wrote: > include url/origin.h Done.
https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... File content/browser/loader/resource_request_info_impl.h (right): https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:111: ResourceRequesterInfo& requester_info() const { return *requester_info_; } On 2016/11/22 01:12:07, horo wrote: > On 2016/11/21 19:51:55, mmenke wrote: > > const methods returning non-const references seems a little weird. I think > this > > can just return a const reference? > > Reflecting on kinuko's opinion, I changed ResourceRequesterInfo::filter() to > non-const. > So this method must return non-const. > > https://codereview.chromium.org/2481093003/diff2/560001:600001/content/browse... > > basically we shouldn't return non-const pointers from const methods (though > some > > of the existing storage context getters below from RMF were not like that but > > it'd be nice if we fix it for the parts that don't require heavy lifting) Then this method should be non-const too, right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/22 02:44:47, kinuko wrote: > https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... > File content/browser/loader/resource_request_info_impl.h (right): > > https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... > content/browser/loader/resource_request_info_impl.h:111: ResourceRequesterInfo& > requester_info() const { return *requester_info_; } > On 2016/11/22 01:12:07, horo wrote: > > On 2016/11/21 19:51:55, mmenke wrote: > > > const methods returning non-const references seems a little weird. I think > > this > > > can just return a const reference? > > > > Reflecting on kinuko's opinion, I changed ResourceRequesterInfo::filter() to > > non-const. > > So this method must return non-const. > > > > > https://codereview.chromium.org/2481093003/diff2/560001:600001/content/browse... > > > basically we shouldn't return non-const pointers from const methods (though > > some > > > of the existing storage context getters below from RMF were not like that > but > > > it'd be nice if we fix it for the parts that don't require heavy lifting) > > Then this method should be non-const too, right? done
The CQ bit was checked by horo@chromium.org to run a CQ dry run
On 2016/11/22 02:44:47, kinuko wrote: > https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... > File content/browser/loader/resource_request_info_impl.h (right): > > https://codereview.chromium.org/2481093003/diff/600001/content/browser/loader... > content/browser/loader/resource_request_info_impl.h:111: ResourceRequesterInfo& > requester_info() const { return *requester_info_; } > On 2016/11/22 01:12:07, horo wrote: > > On 2016/11/21 19:51:55, mmenke wrote: > > > const methods returning non-const references seems a little weird. I think > > this > > > can just return a const reference? > > > > Reflecting on kinuko's opinion, I changed ResourceRequesterInfo::filter() to > > non-const. > > So this method must return non-const. > > > > > https://codereview.chromium.org/2481093003/diff2/560001:600001/content/browse... > > > basically we shouldn't return non-const pointers from const methods (though > > some > > > of the existing storage context getters below from RMF were not like that > but > > > it'd be nice if we fix it for the parts that don't require heavy lifting) > > Then this method should be non-const too, right? done
Just two things, and I've ready to sign off (Not signig off now due to DEPs). Should also have appcache and service worker experts take a look. https://codereview.chromium.org/2481093003/diff/640001/content/browser/loader... File content/browser/loader/DEPS (right): https://codereview.chromium.org/2481093003/diff/640001/content/browser/loader... content/browser/loader/DEPS:145: "+content/browser/loader/resource_requester_info.h", Need to add an enter to this file for resource_requester_info\.(cc|h), with things to be removed (service_worker, appcache, etc) and things not to be. https://codereview.chromium.org/2481093003/diff/640001/content/browser/loader... File content/browser/loader/resource_request_info_impl.h (right): https://codereview.chromium.org/2481093003/diff/640001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:109: ResourceRequesterInfo& requester_info() { return *requester_info_; } Returning non-const references is forbidden. Return a pointer.
The CQ bit was checked by horo@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/2481093003/diff/640001/content/browser/loader... File content/browser/loader/DEPS (right): https://codereview.chromium.org/2481093003/diff/640001/content/browser/loader... content/browser/loader/DEPS:145: "+content/browser/loader/resource_requester_info.h", On 2016/11/22 17:44:12, mmenke wrote: > Need to add an enter to this file for resource_requester_info\.(cc|h), with > things to be removed (service_worker, appcache, etc) and things not to be. Done. https://codereview.chromium.org/2481093003/diff/640001/content/browser/loader... File content/browser/loader/resource_request_info_impl.h (right): https://codereview.chromium.org/2481093003/diff/640001/content/browser/loader... content/browser/loader/resource_request_info_impl.h:109: ResourceRequesterInfo& requester_info() { return *requester_info_; } On 2016/11/22 17:44:12, mmenke wrote: > Returning non-const references is forbidden. Return a pointer. Done.
horo@chromium.org changed reviewers: + falken@chromium.org, michaeln@chromium.org
michaeln@ Could you please review this CL from the appcache point of view? falken@ Could you please review this CL from the service worker point of view? On 2016/11/22 17:44:13, mmenke wrote: > Just two things, and I've ready to sign off (Not signig off now due to DEPs). > Should also have appcache and service worker experts take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
service worker lgtm https://codereview.chromium.org/2481093003/diff/660001/content/browser/loader... File content/browser/loader/resource_requester_info.h (right): https://codereview.chromium.org/2481093003/diff/660001/content/browser/loader... content/browser/loader/resource_requester_info.h:32: // This class represents the type of esource requester. s/esource/resource
Thank you. https://codereview.chromium.org/2481093003/diff/660001/content/browser/loader... File content/browser/loader/resource_requester_info.h (right): https://codereview.chromium.org/2481093003/diff/660001/content/browser/loader... content/browser/loader/resource_requester_info.h:32: // This class represents the type of esource requester. On 2016/11/24 07:34:44, falken wrote: > s/esource/resource Done.
loader/ LGTM!
https://codereview.chromium.org/2481093003/diff/680001/content/browser/loader... File content/browser/loader/async_revalidation_manager.h (right): https://codereview.chromium.org/2481093003/diff/680001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:40: void BeginAsyncRevalidation(net::URLRequest& for_request, Google C++ style guide requires a reference parameter to be const. https://google.github.io/styleguide/cppguide.html#Reference_Arguments
https://codereview.chromium.org/2481093003/diff/680001/content/browser/loader... File content/browser/loader/async_revalidation_manager.h (right): https://codereview.chromium.org/2481093003/diff/680001/content/browser/loader... content/browser/loader/async_revalidation_manager.h:40: void BeginAsyncRevalidation(net::URLRequest& for_request, On 2016/11/29 03:51:51, yhirano wrote: > Google C++ style guide requires a reference parameter to be const. > https://google.github.io/styleguide/cppguide.html#Reference_Arguments Done.
The CQ bit was checked by horo@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...
lgtm
The CQ bit was unchecked by horo@chromium.org
The CQ bit was checked by horo@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...
michaeln@ is ooo, and the change in appcache_interceptor is trivial. So I'd like to add TBR=michaeln@ and submit this cl.
Description was changed from ========== Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request or Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. BUG=468227,649558 ========== to ========== Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request or Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. BUG=468227,649558 TBR=michaeln@chromium.org ==========
The CQ bit was unchecked by horo@chromium.org
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, falken@chromium.org, mmenke@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2481093003/#ps720001 (title: "fix test failure")
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 horo@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 checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, falken@chromium.org, yhirano@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2481093003/#ps740001 (title: "fix URLLoaderFactoryImplTest/URLLoaderFactoryImplTest.GetFailedResponse2")
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": 740001, "attempt_start_ts": 1480412835678700,
"parent_rev": "97d97a2341b3657d828f4e1b5ae0e5a683a7ee5e", "commit_rev":
"05a2b525cff4fcf0c2b0fc0ceb3d1988e8cd7ec1"}
Message was sent while issue was closed.
Description was changed from ========== Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request or Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. BUG=468227,649558 TBR=michaeln@chromium.org ========== to ========== Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request or Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. BUG=468227,649558 TBR=michaeln@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #18 (id:740001)
Message was sent while issue was closed.
Description was changed from ========== Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request or Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. BUG=468227,649558 TBR=michaeln@chromium.org ========== to ========== Introduce ResourceRequesterInfo to abstract the requester of resource requests. Currently there are three types of resource request. - Normal resource request from renderer process. - Resource request for browser side navigation. (PlzNavigate). - Download request or Page Save request. In ResourceRequestInfoImpl, these type are distinguished by process_type and the existence of ResourceMessageFilter. But it is a bit confusing. For example: - AsyncRevalidationManager is checking only the existence of the filter to check that "the renderer is gone" or "the request is for PlzNavigate". - service_worker_context field is added for PlzNavigate support. And the field is used only when the filter is not available in HandleServiceWorkerLink(). This CL introduces ResourceRequesterInfo to make it easy to distinguish those requester types. BUG=468227,649558 TBR=michaeln@chromium.org Committed: https://crrev.com/227286f7cd60cb0ed8756cd1efefb2d694c9734f Cr-Commit-Position: refs/heads/master@{#434957} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/227286f7cd60cb0ed8756cd1efefb2d694c9734f Cr-Commit-Position: refs/heads/master@{#434957} |
