Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860903006:
Handle webuis when using the network service. (Closed)
DescriptionHandle webuis when using the network service.
Some notes:
-once PlzNavigate and Mojo loading ship, then we can use this code path in production. at that point, URLDataManagerBackend should move to the UI thread which would avoid thread hops
-NavigationURLLoaderNetworkService knows about this scheme, but that should be abstracted out later to support other schemes
BUG=717714
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2860903006
Cr-Commit-Position: refs/heads/master@{#469848}
Committed: https://chromium.googlesource.com/chromium/src/+/8c4edd07d3cf96c44d1995e491f8789a7d38a5fd
Patch Set 1 #Patch Set 2 : use factory #Patch Set 3 : handle compression #Patch Set 4 : handle replacements #Patch Set 5 : handle subresources #
Total comments: 3
Patch Set 6 : nits #
Total comments: 37
Patch Set 7 : review comments #
Total comments: 5
Patch Set 8 : comments and fix analyze #Patch Set 9 : merge #Messages
Total messages: 59 (46 generated)
The CQ bit was checked by jam@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_ozone_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 jam@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 ========== First patch for making webuis load using the network service. For now this only gets trivial pages like chrome://about loading. Some notes: -once PlzNavigate and Mojo loading ship, then we can use this code path in production. at that point, URLDataManagerBackend should move to the UI thread which would avoid thread hops -NavigationURLLoaderNetworkService knows about this scheme, but that should be abstracted out later to support other schemes -I first started by making URLDataManagerBackend implement URLLoaderFactory. However that didn't work because the implementation needed more information, namely the FrameTreeNodeID from the plznavigate request. So I called URLDataManagerBackend directly. I could also create a factory for each frame that has the FTN embedded as part of it. Unimplemented in this cl: -localization replacements -gzip -having the renderer connect to a webui URLLoaderFactory BUG=717714 ========== to ========== First patch for making webuis load using the network service. For now this only gets trivial pages like chrome://about loading. Some notes: -once PlzNavigate and Mojo loading ship, then we can use this code path in production. at that point, URLDataManagerBackend should move to the UI thread which would avoid thread hops -NavigationURLLoaderNetworkService knows about this scheme, but that should be abstracted out later to support other schemes Unimplemented in this cl: -localization replacements -gzip -having the renderer connect to a webui URLLoaderFactory BUG=717714 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_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 jam@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
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_ozone_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 jam@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 ========== First patch for making webuis load using the network service. For now this only gets trivial pages like chrome://about loading. Some notes: -once PlzNavigate and Mojo loading ship, then we can use this code path in production. at that point, URLDataManagerBackend should move to the UI thread which would avoid thread hops -NavigationURLLoaderNetworkService knows about this scheme, but that should be abstracted out later to support other schemes Unimplemented in this cl: -localization replacements -gzip -having the renderer connect to a webui URLLoaderFactory BUG=717714 ========== to ========== First patch for making webuis load using the network service. For now this only gets trivial pages like chrome://about loading. Some notes: -once PlzNavigate and Mojo loading ship, then we can use this code path in production. at that point, URLDataManagerBackend should move to the UI thread which would avoid thread hops -NavigationURLLoaderNetworkService knows about this scheme, but that should be abstracted out later to support other schemes Unimplemented in this cl: -localization replacements -having the renderer connect to a webui URLLoaderFactory BUG=717714 ==========
The CQ bit was checked by jam@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 ========== First patch for making webuis load using the network service. For now this only gets trivial pages like chrome://about loading. Some notes: -once PlzNavigate and Mojo loading ship, then we can use this code path in production. at that point, URLDataManagerBackend should move to the UI thread which would avoid thread hops -NavigationURLLoaderNetworkService knows about this scheme, but that should be abstracted out later to support other schemes Unimplemented in this cl: -localization replacements -having the renderer connect to a webui URLLoaderFactory BUG=717714 ========== to ========== First patch for making webuis load using the network service. For now this only gets trivial pages like chrome://about loading. Some notes: -once PlzNavigate and Mojo loading ship, then we can use this code path in production. at that point, URLDataManagerBackend should move to the UI thread which would avoid thread hops -NavigationURLLoaderNetworkService knows about this scheme, but that should be abstracted out later to support other schemes BUG=717714 ==========
Description was changed from ========== First patch for making webuis load using the network service. For now this only gets trivial pages like chrome://about loading. Some notes: -once PlzNavigate and Mojo loading ship, then we can use this code path in production. at that point, URLDataManagerBackend should move to the UI thread which would avoid thread hops -NavigationURLLoaderNetworkService knows about this scheme, but that should be abstracted out later to support other schemes BUG=717714 ========== to ========== First patch for making webuis load using the network service. For now this only gets trivial pages like chrome://about loading. Some notes: -once PlzNavigate and Mojo loading ship, then we can use this code path in production. at that point, URLDataManagerBackend should move to the UI thread which would avoid thread hops -NavigationURLLoaderNetworkService knows about this scheme, but that should be abstracted out later to support other schemes BUG=717714 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== First patch for making webuis load using the network service. For now this only gets trivial pages like chrome://about loading. Some notes: -once PlzNavigate and Mojo loading ship, then we can use this code path in production. at that point, URLDataManagerBackend should move to the UI thread which would avoid thread hops -NavigationURLLoaderNetworkService knows about this scheme, but that should be abstracted out later to support other schemes BUG=717714 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Handle webuis when using the network service. Some notes: -once PlzNavigate and Mojo loading ship, then we can use this code path in production. at that point, URLDataManagerBackend should move to the UI thread which would avoid thread hops -NavigationURLLoaderNetworkService knows about this scheme, but that should be abstracted out later to support other schemes BUG=717714 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by jam@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...
yzshen@chromium.org changed reviewers: + yzshen@chromium.org
https://codereview.chromium.org/2860903006/diff/120001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2860903006/diff/120001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:227: ResourceRequest* request) { Why is it necessary to have an |request| input?
The CQ bit was checked by jam@chromium.org to run a CQ dry run
https://codereview.chromium.org/2860903006/diff/120001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2860903006/diff/120001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:227: ResourceRequest* request) { On 2017/05/05 18:06:21, yzshen1 wrote: > Why is it necessary to have an |request| input? Not anymore (I used it in earlier patchsets, removing, thanks)
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
scottmg@chromium.org changed reviewers: + scottmg@chromium.org
Good stuff, glad this works relatively well. https://codereview.chromium.org/2860903006/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:3052: commit_data.url_loader_factory = mojo::MessagePipeHandle(); nit; put this in else https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... File content/browser/webui/url_data_manager_backend.h (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/url_data_manager_backend.h:50: CreateProtocolHandler(content::ResourceContext* resource_context, remove content:: https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/url_data_manager_backend.h:124: content::ResourceContext* resource_context); remove content:: https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... File content/browser/webui/web_ui_url_loader_factory.cc (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:36: WebContents* GetWebContentsFromFTNID(int frame_tree_node_id) { Maybe you could move this function somewhere accessible, since we have it here, in c/b/loader, and I had to add it in my CL to NavigationURLLoaderNetworkService too. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:96: ResourceResponseHead head; (This is more a general question, not specific to this CL) Do you have any thoughts on handling creation of these responses? There's a lot of fields in ResourceResponseInfo, and I'm not too clear on what's important when. I had to fabricate one of these for the nav request for sw so I just filled in a few fields that seemed easy to do, but it seems a bit hokey. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:124: base::Unretained(source->source()), path, wc_getter, This was doing a RetainedRef on source before (I think?), is the Unretained() safe here? https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:144: uint32_t GetUnGzippedSize( Could you use third_party/zlib/google/compression_utils instead of doing these inline here? https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:156: z_stream inflateStream; inflateStream -> inflate_stream https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:163: CHECK(inflateInit2(&inflateStream, 16) == Z_OK); Where'd the 16 come from? https://cs.chromium.org/chromium/src/net/filter/gzip_source_stream.cc?rcl=9a0... uses -15 I think. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:174: if (replacements_) { Is it going to do this on all response types? Should it be limited to text/html or something in case whatever our template expansion characters are appear in a .png? https://codereview.chromium.org/2860903006/diff/140001/content/child/request_... File content/child/request_extra_data.h (right): https://codereview.chromium.org/2860903006/diff/140001/content/child/request_... content/child/request_extra_data.h:171: mojom::URLLoaderFactory* url_loader_factory_override() const { Remove the content/child files as they're in the other CL.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
Not a review (And don't pan on doing one, deferring to other reviewers here), just wanted to point out a few things. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/url_data_manager_backend.cc:391: return ViewHttpCacheJobFactory::CreateJobForRequest(request, None of these magic chrome URLs are hooked up, right? Just want to make sure you're aware that all the ones here work very differently, so before this can ship, these would need to be handled, too. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... File content/browser/webui/web_ui_url_loader_factory.cc (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:25: #include "net/base/io_buffer.h" Is this used? https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:168: void DataAvailable(scoped_refptr<base::RefCountedMemory> bytes) { Is this a memory mapped file? I seem to recall it is, so we shouldn't be copying from it on a named thread. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:208: BeginWriteDataRaw(data_pipe.producer_handle.get(), &buffer, &num_bytes, Is this guaranteed to work? Is output_sized guaranteed to be something reasonable to fit into browser memory at once?
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
https://codereview.chromium.org/2860903006/diff/120001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2860903006/diff/120001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:3052: commit_data.url_loader_factory = mojo::MessagePipeHandle(); nit: this line could be removed because that is the default value when commit_data is initialized. https://codereview.chromium.org/2860903006/diff/140001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:209: mojom::URLLoaderFactoryPtr factory_ptr; This |factory_ptr| will be destroyed when it goes out of scope. Because |url_loader_associated_ptr_| is associated with it, it will be disconnected as well. That means NavigationURLLoaderNetworkService::FollowRedirect() won't work as expected, the |url_loader_associated_ptr_| will silently drop calls. But I noticed that the URLLoaderImpl for web UI doesn't expect to receive FollowRedirect calls anyway. Maybe it is fine to leave it as it is, but add a comment so that people won't be surprised if they later want to generalize this logic to deal with other schemes. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... File content/browser/webui/web_ui_url_loader_factory.cc (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:63: // NOTE: This duplicates code in URLDataManagerBackend::StartRequest In this case do we also want to add a comment in URLDataManagerBackend::StartRequest, so that people making changes to it also aware of this file? https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:102: mojom::DownloadedTempFilePtr()); nit, fyi: you could use nullptr instead of mojom::DownloadedTempFilePtr().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
jam@chromium.org changed reviewers: + isherman@chromium.org
Thanks all, addressed all comments. +isherman for third_party/zlib/google https://codereview.chromium.org/2860903006/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:3052: commit_data.url_loader_factory = mojo::MessagePipeHandle(); On 2017/05/05 19:27:42, scottmg wrote: > nit; put this in else Done. https://codereview.chromium.org/2860903006/diff/140001/content/browser/loader... File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/loader... content/browser/loader/navigation_url_loader_network_service.cc:209: mojom::URLLoaderFactoryPtr factory_ptr; On 2017/05/05 19:54:07, yzshen1 wrote: > This |factory_ptr| will be destroyed when it goes out of scope. Because > |url_loader_associated_ptr_| is associated with it, it will be disconnected as > well. That means NavigationURLLoaderNetworkService::FollowRedirect() won't work > as expected, the |url_loader_associated_ptr_| will silently drop calls. > > But I noticed that the URLLoaderImpl for web UI doesn't expect to receive > FollowRedirect calls anyway. Maybe it is fine to leave it as it is, but add a > comment so that people won't be surprised if they later want to generalize this > logic to deal with other schemes. Thanks, I didn't realize this. Added comment. agree that we'll have to beef this up once appcache/SW are using something like this. I didn't want to generalize this in this patchset, as it had enough in it. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... File content/browser/webui/url_data_manager_backend.cc (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/url_data_manager_backend.cc:391: return ViewHttpCacheJobFactory::CreateJobForRequest(request, On 2017/05/05 19:34:14, mmenke wrote: > None of these magic chrome URLs are hooked up, right? Just want to make sure > you're aware that all the ones here work very differently, so before this can > ship, these would need to be handled, too. yep, this (and other webui schemes) are on a post it note on my keyboard :) I figured this is a good point for the first patch. Shipping this is a long ways off, as it's blocked on plznavigate, and then that switching to a data pipe. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... File content/browser/webui/url_data_manager_backend.h (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/url_data_manager_backend.h:50: CreateProtocolHandler(content::ResourceContext* resource_context, On 2017/05/05 19:27:42, scottmg wrote: > remove content:: Done. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/url_data_manager_backend.h:124: content::ResourceContext* resource_context); On 2017/05/05 19:27:42, scottmg wrote: > remove content:: Done. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... File content/browser/webui/web_ui_url_loader_factory.cc (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:25: #include "net/base/io_buffer.h" On 2017/05/05 19:34:15, mmenke wrote: > Is this used? nope, removed https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:36: WebContents* GetWebContentsFromFTNID(int frame_tree_node_id) { On 2017/05/05 19:27:42, scottmg wrote: > Maybe you could move this function somewhere accessible, since we have it here, > in c/b/loader, and I had to add it in my CL to NavigationURLLoaderNetworkService > too. sure, reused WebContents::FromFrameTreeNodeId https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:63: // NOTE: This duplicates code in URLDataManagerBackend::StartRequest On 2017/05/05 19:54:07, yzshen1 wrote: > In this case do we also want to add a comment in > URLDataManagerBackend::StartRequest, so that people making changes to it also > aware of this file? Done. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:96: ResourceResponseHead head; On 2017/05/05 19:27:42, scottmg wrote: > (This is more a general question, not specific to this CL) > > Do you have any thoughts on handling creation of these responses? There's a lot > of fields in ResourceResponseInfo, and I'm not too clear on what's important > when. I had to fabricate one of these for the nav request for sw so I just > filled in a few fields that seemed easy to do, but it seems a bit hokey. I agree, it's not clear to me that for webui many of them matter, but probably I'm missing some. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:102: mojom::DownloadedTempFilePtr()); On 2017/05/05 19:54:07, yzshen1 wrote: > nit, fyi: you could use nullptr instead of mojom::DownloadedTempFilePtr(). Done. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:124: base::Unretained(source->source()), path, wc_getter, On 2017/05/05 19:27:42, scottmg wrote: > This was doing a RetainedRef on source before (I think?), is the Unretained() > safe here? Thanks, I missed that in the other code path. Fixed. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:144: uint32_t GetUnGzippedSize( On 2017/05/05 19:27:42, scottmg wrote: > Could you use third_party/zlib/google/compression_utils instead of doing these > inline here? Ah, didn't know about that (this came from the old code that used to ungzip in webui). I'll tweak the third_party versions a bit to avoid extra copies. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:156: z_stream inflateStream; On 2017/05/05 19:27:42, scottmg wrote: > inflateStream -> inflate_stream (removed this code per your other comment) https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:168: void DataAvailable(scoped_refptr<base::RefCountedMemory> bytes) { On 2017/05/05 19:34:15, mmenke wrote: > Is this a memory mapped file? I seem to recall it is, so we shouldn't be > copying from it on a named thread. This is what the other code does on the IO thread as well. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:174: if (replacements_) { On 2017/05/05 19:27:42, scottmg wrote: > Is it going to do this on all response types? Should it be limited to text/html > or something in case whatever our template expansion characters are appear in a > .png? Good point, the other code did this too. I didn't think code handling non-html types would use this, but good to guard the same way. https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:208: BeginWriteDataRaw(data_pipe.producer_handle.get(), &buffer, &num_bytes, On 2017/05/05 19:34:15, mmenke wrote: > Is this guaranteed to work? Is output_sized guaranteed to be something > reasonable to fit into browser memory at once? Good question, I wondered about that as well, but then I realized: -the other code used to decompress at once -the sizes in https://bugs.chromium.org/p/chromium/issues/detail?id=609219 show the bigggest as 1.27M (granted, from a year ago), with the only a few at a few houndred KB and the rest XX KB. -the default data pipe/resourcebuffer size is half a meg So I figured we're safe for now, and if if it's wrong then the CHECK will alert us. https://codereview.chromium.org/2860903006/diff/140001/content/child/request_... File content/child/request_extra_data.h (right): https://codereview.chromium.org/2860903006/diff/140001/content/child/request_... content/child/request_extra_data.h:171: mojom::URLLoaderFactory* url_loader_factory_override() const { On 2017/05/05 19:27:43, scottmg wrote: > Remove the content/child files as they're in the other CL. yep will merge when it lands
The CQ bit was checked by jam@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 jam@chromium.org to run a CQ dry run
Patchset #7 (id:160001) has been deleted
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
lgtm https://codereview.chromium.org/2860903006/diff/180001/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2860903006/diff/180001/content/browser/BUILD.... content/browser/BUILD.gn:1513: "webui/web_ui_url_loader_factory.h", Probably need a deps on zlib here now. https://codereview.chromium.org/2860903006/diff/180001/content/browser/webui/... File content/browser/webui/url_data_manager_backend.h (left): https://codereview.chromium.org/2860903006/diff/180001/content/browser/webui/... content/browser/webui/url_data_manager_backend.h:45: // Invoked to create the protocol handler for chrome://. |is_incognito| should Just to confirm, the is_incognito was being passed through everywhere, but never used, so you just cleaned it up, right? https://codereview.chromium.org/2860903006/diff/180001/content/browser/webui/... File content/browser/webui/web_ui_url_loader_factory.cc (right): https://codereview.chromium.org/2860903006/diff/180001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:30: namespace { super nit; \n after opening { to match the blank line before the closing }
zlib lgtm
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... File content/browser/webui/web_ui_url_loader_factory.cc (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:168: void DataAvailable(scoped_refptr<base::RefCountedMemory> bytes) { On 2017/05/05 22:19:40, jam wrote: > On 2017/05/05 19:34:15, mmenke wrote: > > Is this a memory mapped file? I seem to recall it is, so we shouldn't be > > copying from it on a named thread. > > This is what the other code does on the IO thread as well. I don't think it does. See URLRequestChromeJob::PostReadTask / CopyData (Which is run on a separate thread).
https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... File content/browser/webui/web_ui_url_loader_factory.cc (right): https://codereview.chromium.org/2860903006/diff/140001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:168: void DataAvailable(scoped_refptr<base::RefCountedMemory> bytes) { On 2017/05/05 22:59:27, mmenke wrote: > On 2017/05/05 22:19:40, jam wrote: > > On 2017/05/05 19:34:15, mmenke wrote: > > > Is this a memory mapped file? I seem to recall it is, so we shouldn't be > > > copying from it on a named thread. > > > > This is what the other code does on the IO thread as well. > > I don't think it does. See URLRequestChromeJob::PostReadTask / CopyData (Which > is run on a separate thread). ah, thanks I misread that code then. I'll take care of this in a followup. https://codereview.chromium.org/2860903006/diff/180001/content/browser/webui/... File content/browser/webui/url_data_manager_backend.h (left): https://codereview.chromium.org/2860903006/diff/180001/content/browser/webui/... content/browser/webui/url_data_manager_backend.h:45: // Invoked to create the protocol handler for chrome://. |is_incognito| should On 2017/05/05 22:38:10, scottmg wrote: > Just to confirm, the is_incognito was being passed through everywhere, but never > used, so you just cleaned it up, right? Right https://codereview.chromium.org/2860903006/diff/180001/content/browser/webui/... File content/browser/webui/web_ui_url_loader_factory.cc (right): https://codereview.chromium.org/2860903006/diff/180001/content/browser/webui/... content/browser/webui/web_ui_url_loader_factory.cc:30: namespace { On 2017/05/05 22:38:10, scottmg wrote: > super nit; \n after opening { to match the blank line before the closing } Done.
The CQ bit was checked by jam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2860903006/#ps220001 (title: "merge")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1494028198337760,
"parent_rev": "fc104446e6a2c11167e7f6e02b944f7e752be8b7", "commit_rev":
"8c4edd07d3cf96c44d1995e491f8789a7d38a5fd"}
Message was sent while issue was closed.
Description was changed from ========== Handle webuis when using the network service. Some notes: -once PlzNavigate and Mojo loading ship, then we can use this code path in production. at that point, URLDataManagerBackend should move to the UI thread which would avoid thread hops -NavigationURLLoaderNetworkService knows about this scheme, but that should be abstracted out later to support other schemes BUG=717714 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Handle webuis when using the network service. Some notes: -once PlzNavigate and Mojo loading ship, then we can use this code path in production. at that point, URLDataManagerBackend should move to the UI thread which would avoid thread hops -NavigationURLLoaderNetworkService knows about this scheme, but that should be abstracted out later to support other schemes BUG=717714 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2860903006 Cr-Commit-Position: refs/heads/master@{#469848} Committed: https://chromium.googlesource.com/chromium/src/+/8c4edd07d3cf96c44d1995e491f8... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:220001) as https://chromium.googlesource.com/chromium/src/+/8c4edd07d3cf96c44d1995e491f8... |
