Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2410333006:
Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). (Closed)
|
Created:
4 years, 2 months ago by horo Modified:
4 years, 2 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, cbentzel+watch_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, eroman, kinuko+serviceworker, horo+watch_chromium.org, darin-cc_chromium.org, kinuko+watch, tzik, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload().
This CL depends on https://codereview.chromium.org/2417793002.
FetchEvent.navigationPreload will be implemented in the following steps.
[1/4] Introduce FetchEventPreloadHandle to pass the preload response to FetchEvent.
https://codereview.chromium.org/2417793002/
[2/4] Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload().
https://codereview.chromium.org/2410333006/ This CL.
[3/4] Implement FetchEvent.navigationPreload.
https://codereview.chromium.org/2416843002/
[4/4] Add browser tests for NavigationPreload.
https://codereview.chromium.org/2413643005/
BUG=649558
Committed: https://crrev.com/36762af751e2100946cff85db993910e8a402423
Cr-Commit-Position: refs/heads/master@{#426423}
Patch Set 1 : with https://codereview.chromium.org/2410333006/ #Patch Set 2 : diff from with https://codereview.chromium.org/2410333006/ #
Total comments: 8
Patch Set 3 : add comment #Patch Set 4 : incorporated falken's comment #
Total comments: 6
Patch Set 5 : incorporated mmenke's comment #Patch Set 6 : introduce RedirectionBlockingResourceHandler #Patch Set 7 : go through the mojo URLLoaderFactory interface #Patch Set 8 : add comment about switch #
Total comments: 6
Patch Set 9 : incorporated falken's comment #
Total comments: 2
Patch Set 10 : add comment in sw_fetch_dispatcher.cc and split the media test change to 2427363004 #
Total comments: 6
Patch Set 11 : DCHECK_CURRENTLY_ON(BrowserThread::IO) in MakeRequestID() #Patch Set 12 : incorporated sadrul's comment #
Total comments: 3
Patch Set 13 : add comment in service_worker_url_request_job.h #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 88 (48 generated)
Description was changed from ========== Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). BUG=649558 ========== to ========== Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). This CL depends on https://codereview.chromium.org/2417793002. BUG=649558 ==========
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Description was changed from ========== Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). This CL depends on https://codereview.chromium.org/2417793002. BUG=649558 ========== to ========== Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). This CL depends on https://codereview.chromium.org/2417793002. FetchEvent.navigationPreload will be implemented in the following steps. [1/4] Introduce FetchEventPreloadHandle to pass the preload response to FetchEvent. https://codereview.chromium.org/2417793002/ [2/4] Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). https://codereview.chromium.org/2410333006/ This CL. [3/4] Implement FetchEvent.navigationPreload. https://codereview.chromium.org/2416843002/ [4/4] Add browser tests for NavigationPreload. https://codereview.chromium.org/2413643005/ BUG=649558 ==========
Patchset #1 (id:1) 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...
horo@chromium.org changed reviewers: + falken@chromium.org
falken@ Could you please review this?
This lgtm I think you should get a /net person to review the LayeredResourceHandler stuff since I'm not too familiar with it. https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... content/browser/service_worker/service_worker_fetch_dispatcher.cc:283: // Currently NavigationPreload doesn't support request body. nit: TODO(horo)? https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_navigation_preload_fetcher.cc (right): https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... content/browser/service_worker/service_worker_navigation_preload_fetcher.cc:55: net::NetLogEventType::SERVICE_WORKER_NAVIGATION_PRELOAD); Could be useful to log status. https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... content/browser/service_worker/service_worker_navigation_preload_fetcher.cc:56: LayeredResourceHandler::OnResponseCompleted(status, defer); Is OnResponseCompleted definitely called for every OnWillStart? Might it make sense to put the Begin/End in the ctor and dtor? https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_navigation_preload_fetcher.h (right): https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... content/browser/service_worker/service_worker_navigation_preload_fetcher.h:16: class ServiceWorkerNavigationPreloadFetcher { Add a lightweight comment about what this class does?
Patchset #4 (id:80001) has been deleted
horo@chromium.org changed reviewers: + mmenke@chromium.org
mmenke@ Could you please review this CL? https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... content/browser/service_worker/service_worker_fetch_dispatcher.cc:283: // Currently NavigationPreload doesn't support request body. On 2016/10/14 07:53:12, falken wrote: > nit: TODO(horo)? Done. https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_navigation_preload_fetcher.cc (right): https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... content/browser/service_worker/service_worker_navigation_preload_fetcher.cc:55: net::NetLogEventType::SERVICE_WORKER_NAVIGATION_PRELOAD); On 2016/10/14 07:53:12, falken wrote: > Could be useful to log status. Done. https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... content/browser/service_worker/service_worker_navigation_preload_fetcher.cc:56: LayeredResourceHandler::OnResponseCompleted(status, defer); On 2016/10/14 07:53:12, falken wrote: > Is OnResponseCompleted definitely called for every OnWillStart? Might it make > sense to put the Begin/End in the ctor and dtor? Done. Sounds good. OnWillStart() is not called if the request is cancelled. https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_navigation_preload_fetcher.h (right): https://codereview.chromium.org/2410333006/diff/40001/content/browser/service... content/browser/service_worker/service_worker_navigation_preload_fetcher.h:16: class ServiceWorkerNavigationPreloadFetcher { On 2016/10/14 07:53:12, falken wrote: > Add a lightweight comment about what this class does? Done.
+CC:yhirano@
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/2410333006/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_navigation_preload_fetcher.cc (right): https://codereview.chromium.org/2410333006/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_navigation_preload_fetcher.cc:35: class PreloadResourceHandler : public LayeredResourceHandler { While this class is really simple (Which is good), the ResourceHandler API is pretty horrible, and I'd rather not make another one unless we really need to. I don't think we have any ResourceHandlers that exist only for logging purposes. Is this just to tag the request with SERVICE_WORKER_NAVIGATION_PRELOAD for tracing? If the purpose is just net-internals, I'd suggest adding a singleton event, with not end event. https://codereview.chromium.org/2410333006/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_navigation_preload_fetcher.cc:90: preload_handle->url_loader_client_request = GetProxy(&url_loader_client); What's going on here? What class is driving the url_loader_client, any why can't it use the main mojo URLRequest factory thing? https://codereview.chromium.org/2410333006/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_navigation_preload_fetcher.cc:92: std::unique_ptr<ResourceHandler> handler(new MojoAsyncResourceHandler( MojoAsyncResourceHandler isn't ready for prime time yet - it doesn't support redirects, among many other things. Not handling meaning it DCHECKs. I'm also not really sure we should be creating them outside RDH.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
horo@chromium.org changed reviewers: + yhirano@chromium.org
https://codereview.chromium.org/2410333006/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_navigation_preload_fetcher.cc (right): https://codereview.chromium.org/2410333006/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_navigation_preload_fetcher.cc:35: class PreloadResourceHandler : public LayeredResourceHandler { On 2016/10/14 16:31:33, mmenke wrote: > While this class is really simple (Which is good), the ResourceHandler API is > pretty horrible, and I'd rather not make another one unless we really need to. > I don't think we have any ResourceHandlers that exist only for logging purposes. > > Is this just to tag the request with SERVICE_WORKER_NAVIGATION_PRELOAD for > tracing? If the purpose is just net-internals, I'd suggest adding a singleton > event, with not end event. Done. https://codereview.chromium.org/2410333006/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_navigation_preload_fetcher.cc:90: preload_handle->url_loader_client_request = GetProxy(&url_loader_client); On 2016/10/14 16:31:33, mmenke wrote: > What's going on here? What class is driving the url_loader_client, any why > can't it use the main mojo URLRequest factory thing? This preload_handle will be passed to the service worker in the renderer process. https://codereview.chromium.org/2416843002/ implements the renderer process side logic. NavigationRequests are initiated by the browser process, not the renderer process. It must be started even if the service worker is not started yet. So the browser process starts the request here. The browser process creates URLLoaderClientRequest here and pass it to the service worker in the renderer process after starting the service worker. https://codereview.chromium.org/2417793002/diff/60001/content/browser/service... When the service worker receives the URLLoaderClientRequest, it creates the URLLoaderClient which will drive the request. https://codereview.chromium.org/2416843002/diff/110001/content/renderer/servi... https://codereview.chromium.org/2410333006/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_navigation_preload_fetcher.cc:92: std::unique_ptr<ResourceHandler> handler(new MojoAsyncResourceHandler( On 2016/10/14 16:31:33, mmenke wrote: > MojoAsyncResourceHandler isn't ready for prime time yet - it doesn't support > redirects, among many other things. Not handling meaning it DCHECKs. I'm also > not really sure we should be creating them outside RDH. NavigationPreload is still an experimental feature. And we don't need to support redirects. I think there is no problem to use MojoAsyncResourceHandler for NavigationPreloads. yhirano@ What do you think as the author of MojoAsyncResourceHandler?
On 2016/10/15 00:55:46, horo wrote: > https://codereview.chromium.org/2410333006/diff/100001/content/browser/servic... > File content/browser/service_worker/service_worker_navigation_preload_fetcher.cc > (right): > > https://codereview.chromium.org/2410333006/diff/100001/content/browser/servic... > content/browser/service_worker/service_worker_navigation_preload_fetcher.cc:35: > class PreloadResourceHandler : public LayeredResourceHandler { > On 2016/10/14 16:31:33, mmenke wrote: > > While this class is really simple (Which is good), the ResourceHandler API is > > pretty horrible, and I'd rather not make another one unless we really need to. > > > I don't think we have any ResourceHandlers that exist only for logging > purposes. > > > > Is this just to tag the request with SERVICE_WORKER_NAVIGATION_PRELOAD for > > tracing? If the purpose is just net-internals, I'd suggest adding a singleton > > event, with not end event. > > Done. > > https://codereview.chromium.org/2410333006/diff/100001/content/browser/servic... > content/browser/service_worker/service_worker_navigation_preload_fetcher.cc:90: > preload_handle->url_loader_client_request = GetProxy(&url_loader_client); > On 2016/10/14 16:31:33, mmenke wrote: > > What's going on here? What class is driving the url_loader_client, any why > > can't it use the main mojo URLRequest factory thing? > > This preload_handle will be passed to the service worker in the renderer > process. > https://codereview.chromium.org/2416843002/ implements the renderer process side > logic. > > NavigationRequests are initiated by the browser process, not the renderer > process. > It must be started even if the service worker is not started yet. > > So the browser process starts the request here. > The browser process creates URLLoaderClientRequest here and pass it to the > service worker in the renderer process after starting the service worker. > https://codereview.chromium.org/2417793002/diff/60001/content/browser/service... > > When the service worker receives the URLLoaderClientRequest, it creates the > URLLoaderClient which will drive the request. > https://codereview.chromium.org/2416843002/diff/110001/content/renderer/servi... > > https://codereview.chromium.org/2410333006/diff/100001/content/browser/servic... > content/browser/service_worker/service_worker_navigation_preload_fetcher.cc:92: > std::unique_ptr<ResourceHandler> handler(new MojoAsyncResourceHandler( > On 2016/10/14 16:31:33, mmenke wrote: > > MojoAsyncResourceHandler isn't ready for prime time yet - it doesn't support > > redirects, among many other things. Not handling meaning it DCHECKs. I'm > also > > not really sure we should be creating them outside RDH. > > NavigationPreload is still an experimental feature. > And we don't need to support redirects. I don't mean it just cancels on redirect, I mean it hits a NOTREACHED, and there's no way to cancel on redirects before that. It should never be possible to hit a NOTREACHED. > I think there is no problem to use MojoAsyncResourceHandler for > NavigationPreloads. > > yhirano@ > What do you think as the author of MojoAsyncResourceHandler?
Patchset #6 (id:140001) 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...
> I don't mean it just cancels on redirect, I mean it hits a NOTREACHED, and > there's no way to cancel on redirects before that. It should never be possible > to hit a NOTREACHED. I uploaded Patch Set 6 which revives a LayeredResourceHandler to block the redirect response. RedirectionBlockingResourceHandler cancels the redirects before MojoAsyncResourceHandler.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
MojoAsyncResourceHandler is not ready for production use, but this feature is experimental as well IIUC. I would be happy if you could find more bugs by using it.
On 2016/10/17 13:12:29, yhirano wrote: > MojoAsyncResourceHandler is not ready for production use, but this feature is > experimental as well IIUC. I would be happy if you could find more bugs by using > it. Enabling it in experiments, even on Canary and dev, seems to fit under the "production use" rubric, so I'm concerned about landing this CL until then. I'm also concerned about another path creating the MojoResourceHandler - seems like it should be going through the normal mojo path to create one of these, not an alternative path, and I don't understand the reason for it.
On 2016/10/17 16:42:59, mmenke wrote: > Enabling it in experiments, even on Canary and dev, seems to fit under the > "production use" rubric, so I'm concerned about landing this CL until then. This NavigationPreload feature is under the "enable-experimental-web-platform-features" flag. Is it "production use"? > I'm > also concerned about another path creating the MojoResourceHandler - seems like > it should be going through the normal mojo path to create one of these, not an > alternative path, and I don't understand the reason for it. Is "going through the normal mojo path" means calling ResourceDispatcherHostImpl::CreateResourceHandler()? The client of the navigation request is the service worker. But the service worker may not be started yet. So I think we have to start the request in the browser before starting the service worker and pass the URLLoaderClient"Request". Do you have any other idea about how to start the network request before the url request client (= service worker) is not started yet?
On 2016/10/17 17:01:55, horo wrote: > On 2016/10/17 16:42:59, mmenke wrote: > > Enabling it in experiments, even on Canary and dev, seems to fit under the > > "production use" rubric, so I'm concerned about landing this CL until then. > > This NavigationPreload feature is under the > "enable-experimental-web-platform-features" flag. > Is it "production use"? I'd call that production use, since it's group with so many other things, and I suspect a fair number of people have it enabled. If it's also gated on mojo being enabled for normal web requests, I'm fine with it, however. > > I'm > > also concerned about another path creating the MojoResourceHandler - seems > like > > it should be going through the normal mojo path to create one of these, not an > > alternative path, and I don't understand the reason for it. > > > Is "going through the normal mojo path" means calling > ResourceDispatcherHostImpl::CreateResourceHandler()? The "normal path" would mean either going through the mojo URLLoaderFactory interface, or going through ResourceDispatcher. I'm fine with either one. The latter is nice in that it's already in use, but if you're in the browser process, that may be problematic. Also note that if you use the URLLoaderFactory, your code should basically be mojo-ready. If you directly call into the ResourceDispatcherHost, that's not the case. > The client of the navigation request is the service worker. > But the service worker may not be started yet. > So I think we have to start the request in the browser before starting the > service worker and pass the URLLoaderClient"Request". > Do you have any other idea about how to start the network request before the url > request client (= service worker) is not started yet?
> This NavigationPreload feature is under the > "enable-experimental-web-platform-features" flag. > Is it "production use"? "I'd call that production use, since it's group with so many other things, and I suspect a fair number of people have it enabled. If it's also gated on mojo being enabled for normal web requests, I'm fine with it, however." mmenke: Would it change your opinion if NavigationPreload was gated on a separate --enable-navigation-preload flag rather than --enable-experimental-web-platform-features? New/experimental web platform features typically just use the generic flag and this has advantages for testing: Blink layout tests are run with that flag enabled. Note also that NavigationPreload must be opted-in to by the site. If the site doesn’t use the NavigationPreload API this code path won’t be taken. So it requires both (A) the site to use the experimental feature and (B) the user to enable the flag, so it seems likely to have little effect in the wild until we're ready to start an origin trial or launch, which we won't do until it's production ready.
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...
On 2016/10/17 20:00:45, mmenke wrote: > The "normal path" would mean either going through the mojo URLLoaderFactory > interface, or going through ResourceDispatcher. I'm fine with either one. The > latter is nice in that it's already in use, but if you're in the browser > process, that may be problematic. Also note that if you use the > URLLoaderFactory, your code should basically be mojo-ready. If you directly > call into the ResourceDispatcherHost, that's not the case. I uploaded Patch Set 7. The request will go through the mojo URLLoaderFactory interface. Please review it. Thank you.
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 #7 (id:180001) 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: This issue passed the CQ dry run.
On 2016/10/18 07:36:43, falken wrote: > > This NavigationPreload feature is under the > > "enable-experimental-web-platform-features" flag. > > Is it "production use"? > > "I'd call that production use, since it's group with so many other things, and I > suspect a fair number of people have it enabled. If it's also gated on mojo > being enabled for normal web requests, I'm fine with it, however." > > mmenke: Would it change your opinion if NavigationPreload was gated on a > separate --enable-navigation-preload flag rather than > --enable-experimental-web-platform-features? New/experimental web platform > features typically just use the generic flag and this has advantages for > testing: Blink layout tests are run with that flag enabled. I'd be fine with using enable-navigation-preload. Basically, I don't want to have to deal with people running into bugs with MojoAsyncResourceHandler in the field, or worse, release block bugs if it's broken enough that even a small population with it enabled cause it tot > Note also that NavigationPreload must be opted-in to by the site. If the site > doesn’t use the NavigationPreload API this code path won’t be taken. So it > requires both (A) the site to use the experimental feature and (B) the user to > enable the flag, so it seems likely to have little effect in the wild until > we're ready to start an origin trial or launch, which we won't do until it's > production ready. I suspect that a fair number of developers keep experimental web platform features enabled, as they experiment with new features being added to the platform. While it's fine for these features to have bugs, AsyncMojoResourceHandler is broken enough that I don't think it should be included here yet. Using it may have unforseen side effects, break a bunch of APIs, result in sites being effectively blackholed until Chrome is restarted, etc. Investigating these as "bugs" is just a waste of time. It's not ready, we know it's not ready, and I don't want anyone on the network team to spend their time investigating such "bugs" - that's my main concern here.
On 2016/10/18 07:47:49, horo wrote: > On 2016/10/17 20:00:45, mmenke wrote: > > The "normal path" would mean either going through the mojo URLLoaderFactory > > interface, or going through ResourceDispatcher. I'm fine with either one. > The > > latter is nice in that it's already in use, but if you're in the browser > > process, that may be problematic. Also note that if you use the > > URLLoaderFactory, your code should basically be mojo-ready. If you directly > > call into the ResourceDispatcherHost, that's not the case. > > > I uploaded Patch Set 7. > The request will go through the mojo URLLoaderFactory interface. > Please review it. > > Thank you. I didn't look at how the feature is enabled, but PS7 LGTM, otherwise.
On 2016/10/18 14:40:55, mmenke wrote: > On 2016/10/18 07:36:43, falken wrote: > > > This NavigationPreload feature is under the > > > "enable-experimental-web-platform-features" flag. > > > Is it "production use"? > > > > "I'd call that production use, since it's group with so many other things, and > I > > suspect a fair number of people have it enabled. If it's also gated on mojo > > being enabled for normal web requests, I'm fine with it, however." > > > > mmenke: Would it change your opinion if NavigationPreload was gated on a > > separate --enable-navigation-preload flag rather than > > --enable-experimental-web-platform-features? New/experimental web platform > > features typically just use the generic flag and this has advantages for > > testing: Blink layout tests are run with that flag enabled. > > I'd be fine with using enable-navigation-preload. Basically, I don't want to > have to deal with people running into bugs with MojoAsyncResourceHandler in the > field, or worse, release block bugs if it's broken enough that even a small > population with it enabled cause it tot > > > Note also that NavigationPreload must be opted-in to by the site. If the site > > doesn’t use the NavigationPreload API this code path won’t be taken. So it > > requires both (A) the site to use the experimental feature and (B) the user to > > enable the flag, so it seems likely to have little effect in the wild until > > we're ready to start an origin trial or launch, which we won't do until it's > > production ready. > > I suspect that a fair number of developers keep experimental web platform > features enabled, as they experiment with new features being added to the > platform. While it's fine for these features to have bugs, > AsyncMojoResourceHandler is broken enough that I don't think it should be > included here yet. Using it may have unforseen side effects, break a bunch of > APIs, result in sites being effectively blackholed until Chrome is restarted, > etc. Investigating these as "bugs" is just a waste of time. It's not ready, we > know it's not ready, and I don't want anyone on the network team to spend their > time investigating such "bugs" - that's my main concern here. Makes sense. horo@ can we enable this feature only when both experimental-web-platform-features and LoadingWithMojo are enabled, or should we do a new flag --enable-navigation-preload?
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...
On 2016/10/19 00:11:57, falken wrote: > On 2016/10/18 14:40:55, mmenke wrote: > > On 2016/10/18 07:36:43, falken wrote: > > > > This NavigationPreload feature is under the > > > > "enable-experimental-web-platform-features" flag. > > > > Is it "production use"? > > > > > > "I'd call that production use, since it's group with so many other things, > and > > I > > > suspect a fair number of people have it enabled. If it's also gated on mojo > > > being enabled for normal web requests, I'm fine with it, however." > > > > > > mmenke: Would it change your opinion if NavigationPreload was gated on a > > > separate --enable-navigation-preload flag rather than > > > --enable-experimental-web-platform-features? New/experimental web platform > > > features typically just use the generic flag and this has advantages for > > > testing: Blink layout tests are run with that flag enabled. > > > > I'd be fine with using enable-navigation-preload. Basically, I don't want to > > have to deal with people running into bugs with MojoAsyncResourceHandler in > the > > field, or worse, release block bugs if it's broken enough that even a small > > population with it enabled cause it tot > > > > > Note also that NavigationPreload must be opted-in to by the site. If the > site > > > doesn’t use the NavigationPreload API this code path won’t be taken. So it > > > requires both (A) the site to use the experimental feature and (B) the user > to > > > enable the flag, so it seems likely to have little effect in the wild until > > > we're ready to start an origin trial or launch, which we won't do until it's > > > production ready. > > > > I suspect that a fair number of developers keep experimental web platform > > features enabled, as they experiment with new features being added to the > > platform. While it's fine for these features to have bugs, > > AsyncMojoResourceHandler is broken enough that I don't think it should be > > included here yet. Using it may have unforseen side effects, break a bunch of > > APIs, result in sites being effectively blackholed until Chrome is restarted, > > etc. Investigating these as "bugs" is just a waste of time. It's not ready, > we > > know it's not ready, and I don't want anyone on the network team to spend > their > > time investigating such "bugs" - that's my main concern here. > > Makes sense. horo@ can we enable this feature only when both > experimental-web-platform-features and LoadingWithMojo are enabled, or should we > do a new flag --enable-navigation-preload? I think we should introduce a new flag --enable-service-worker-navigation-preload. I added a comment about it (PatchSet8). And I will create a CL to introduce the new flag.
falken@ Could you please review PS8 again? There are differences after PS4.
On 2016/10/19 03:15:13, horo wrote: > On 2016/10/19 00:11:57, falken wrote: > > On 2016/10/18 14:40:55, mmenke wrote: > > > On 2016/10/18 07:36:43, falken wrote: > > > > > This NavigationPreload feature is under the > > > > > "enable-experimental-web-platform-features" flag. > > > > > Is it "production use"? > > > > > > > > "I'd call that production use, since it's group with so many other things, > > and > > > I > > > > suspect a fair number of people have it enabled. If it's also gated on > mojo > > > > being enabled for normal web requests, I'm fine with it, however." > > > > > > > > mmenke: Would it change your opinion if NavigationPreload was gated on a > > > > separate --enable-navigation-preload flag rather than > > > > --enable-experimental-web-platform-features? New/experimental web platform > > > > features typically just use the generic flag and this has advantages for > > > > testing: Blink layout tests are run with that flag enabled. > > > > > > I'd be fine with using enable-navigation-preload. Basically, I don't want > to > > > have to deal with people running into bugs with MojoAsyncResourceHandler in > > the > > > field, or worse, release block bugs if it's broken enough that even a small > > > population with it enabled cause it tot > > > > > > > Note also that NavigationPreload must be opted-in to by the site. If the > > site > > > > doesn’t use the NavigationPreload API this code path won’t be taken. So it > > > > requires both (A) the site to use the experimental feature and (B) the > user > > to > > > > enable the flag, so it seems likely to have little effect in the wild > until > > > > we're ready to start an origin trial or launch, which we won't do until > it's > > > > production ready. > > > > > > I suspect that a fair number of developers keep experimental web platform > > > features enabled, as they experiment with new features being added to the > > > platform. While it's fine for these features to have bugs, > > > AsyncMojoResourceHandler is broken enough that I don't think it should be > > > included here yet. Using it may have unforseen side effects, break a bunch > of > > > APIs, result in sites being effectively blackholed until Chrome is > restarted, > > > etc. Investigating these as "bugs" is just a waste of time. It's not > ready, > > we > > > know it's not ready, and I don't want anyone on the network team to spend > > their > > > time investigating such "bugs" - that's my main concern here. > > > > Makes sense. horo@ can we enable this feature only when both > > experimental-web-platform-features and LoadingWithMojo are enabled, or should > we > > do a new flag --enable-navigation-preload? > > I think we should introduce a new flag > --enable-service-worker-navigation-preload. > I added a comment about it (PatchSet8). > And I will create a CL to introduce the new flag. Created: https://crrev.com/2431003003/
https://codereview.chromium.org/2410333006/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2410333006/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:317: request.resource_type = RESOURCE_TYPE_SUB_RESOURCE; Why SUB_RESOURCE instead of resource_type_? https://codereview.chromium.org/2410333006/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:325: DCHECK(request_id <= -2); nit: DCHECK_LT -1? https://codereview.chromium.org/2410333006/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.h (right): https://codereview.chromium.org/2410333006/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.h:36: base::Callback<bool(mojo::InterfaceRequest<mojom::URLLoaderFactory>)>; Can we move this declaration to service_worker_types.h so it doesn't need to be copy/pasted in multiple classes? It's also not obvious from the name what URLLoaderFactoryCallback does. Maybe add a comment that this creates a URLLoaderFactory?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
The CQ bit was checked by horo@chromium.org to run a CQ dry run
https://codereview.chromium.org/2410333006/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2410333006/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:317: request.resource_type = RESOURCE_TYPE_SUB_RESOURCE; On 2016/10/19 05:11:27, falken wrote: > Why SUB_RESOURCE instead of resource_type_? It is because we shouldn't trigger NavigationResourceThrottle for the SW navigation request. https://codereview.chromium.org/2410333006/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:325: DCHECK(request_id <= -2); On 2016/10/19 05:11:28, falken wrote: > nit: DCHECK_LT -1? Done. https://codereview.chromium.org/2410333006/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.h (right): https://codereview.chromium.org/2410333006/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.h:36: base::Callback<bool(mojo::InterfaceRequest<mojom::URLLoaderFactory>)>; On 2016/10/19 05:11:28, falken wrote: > Can we move this declaration to service_worker_types.h so it doesn't need to be > copy/pasted in multiple classes? > > It's also not obvious from the name what URLLoaderFactoryCallback does. Maybe > add a comment that this creates a URLLoaderFactory? Done.
lgtm https://codereview.chromium.org/2410333006/diff/240001/content/browser/servic... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://codereview.chromium.org/2410333006/diff/240001/content/browser/servic... content/browser/service_worker/service_worker_fetch_dispatcher.cc:318: request.resource_type = RESOURCE_TYPE_SUB_RESOURCE; Let's add a comment like what you said: Set to SUB_RESOURCE because we shouldn't trigger NavigationResourceThrottle for the SW navigation request.
Patchset #10 (id:260001) has been deleted
Thank you. https://chromiumcodereview.appspot.com/2410333006/diff/240001/content/browser... File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right): https://chromiumcodereview.appspot.com/2410333006/diff/240001/content/browser... content/browser/service_worker/service_worker_fetch_dispatcher.cc:318: request.resource_type = RESOURCE_TYPE_SUB_RESOURCE; On 2016/10/19 06:43:18, falken wrote: > Let's add a comment like what you said: > Set to SUB_RESOURCE because we shouldn't trigger NavigationResourceThrottle for > the SW navigation request. Done.
horo@chromium.org changed reviewers: + dcheng@chromium.org, sadrul@chromium.org
sadrul@ Please review content/browser/renderer_host/render_process_host_impl.* dcheng@ Could you please review this change as a security reviewer? This CL is the browser process side implementation of FetchEvent.navigationPreload.
I have a question: I am making a change to MojoAsyncResourceHandler: It will interact with associated interface pointers rather than interface pointers in order to getting along with existing IPCs. Is that problematic for you?
https://codereview.chromium.org/2410333006/diff/280001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2410333006/diff/280001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3016: mojo::InterfaceRequest<mojom::URLLoaderFactory> request) { This doesn't need to be a static, right?
https://codereview.chromium.org/2410333006/diff/280001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2410333006/diff/280001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3016: mojo::InterfaceRequest<mojom::URLLoaderFactory> request) { On 2016/10/19 14:42:43, sadrul wrote: > This doesn't need to be a static, right? I want to return false if RenderProcessHostImpl was deleted. So I think this method must be static.
My signoff still stands, just hadn't thought about thread safety (Which looks fine in this CL, just seems safest to have a DCHECK against future misuse) https://codereview.chromium.org/2410333006/diff/280001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2410333006/diff/280001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:342: int MakeRequestID() { return --request_id_; } Could you make this DCHECK that we're on the IO thread, and move it to the cc file?
https://codereview.chromium.org/2410333006/diff/280001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2410333006/diff/280001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3016: mojo::InterfaceRequest<mojom::URLLoaderFactory> request) { On 2016/10/19 14:48:41, horo wrote: > On 2016/10/19 14:42:43, sadrul wrote: > > This doesn't need to be a static, right? > > I want to return false if RenderProcessHostImpl was deleted. > So I think this method must be static. I think the return type can be void, and you can check if the callback has been canceled (using base::Callback::IsCanceled()) instead.
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/2410333006/diff/280001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2410333006/diff/280001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:342: int MakeRequestID() { return --request_id_; } On 2016/10/19 14:56:02, mmenke wrote: > Could you make this DCHECK that we're on the IO thread, and move it to the cc > file? Done.
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...
https://codereview.chromium.org/2410333006/diff/280001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2410333006/diff/280001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:3016: mojo::InterfaceRequest<mojom::URLLoaderFactory> request) { On 2016/10/19 15:13:31, sadrul wrote: > On 2016/10/19 14:48:41, horo wrote: > > On 2016/10/19 14:42:43, sadrul wrote: > > > This doesn't need to be a static, right? > > > > I want to return false if RenderProcessHostImpl was deleted. > > So I think this method must be static. > > I think the return type can be void, and you can check if the callback has been > canceled (using base::Callback::IsCanceled()) instead. Ah, sounds good. Done.
On 2016/10/19 12:03:41, yhirano wrote: > I have a question: I am making a change to MojoAsyncResourceHandler: It will > interact with associated interface pointers rather than interface pointers in > order to getting along with existing IPCs. Is that problematic for you? If my understanding is correct, it is not so problematic, but I have to change this CL to use associated interface pointers.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mojo LGTM https://codereview.chromium.org/2410333006/diff/320001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/2410333006/diff/320001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:344: // |dispatcher_host_| may be null in some unittests. Sad =( https://codereview.chromium.org/2410333006/diff/320001/content/browser/servic... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2410333006/diff/320001/content/browser/servic... content/browser/service_worker/service_worker_url_request_job.h:98: const MojoURLLoaderFactoryGetter& url_loader_factory_getter, For me, it's a little confusing that this is a null callback most of the time. Maybe some sort of comment on the member that indicates when this is used would be helpful for maintainability in the future.
Thank you! https://codereview.chromium.org/2410333006/diff/320001/content/browser/servic... File content/browser/service_worker/service_worker_url_request_job.h (right): https://codereview.chromium.org/2410333006/diff/320001/content/browser/servic... content/browser/service_worker/service_worker_url_request_job.h:98: const MojoURLLoaderFactoryGetter& url_loader_factory_getter, On 2016/10/20 02:35:50, dcheng wrote: > For me, it's a little confusing that this is a null callback most of the time. > Maybe some sort of comment on the member that indicates when this is used would > be helpful for maintainability in the future. Done.
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, falken@chromium.org, sadrul@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2410333006/#ps340001 (title: "add comment in service_worker_url_request_job.h")
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 horo@chromium.org
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...
Message was sent while issue was closed.
Description was changed from ========== Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). This CL depends on https://codereview.chromium.org/2417793002. FetchEvent.navigationPreload will be implemented in the following steps. [1/4] Introduce FetchEventPreloadHandle to pass the preload response to FetchEvent. https://codereview.chromium.org/2417793002/ [2/4] Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). https://codereview.chromium.org/2410333006/ This CL. [3/4] Implement FetchEvent.navigationPreload. https://codereview.chromium.org/2416843002/ [4/4] Add browser tests for NavigationPreload. https://codereview.chromium.org/2413643005/ BUG=649558 ========== to ========== Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). This CL depends on https://codereview.chromium.org/2417793002. FetchEvent.navigationPreload will be implemented in the following steps. [1/4] Introduce FetchEventPreloadHandle to pass the preload response to FetchEvent. https://codereview.chromium.org/2417793002/ [2/4] Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). https://codereview.chromium.org/2410333006/ This CL. [3/4] Implement FetchEvent.navigationPreload. https://codereview.chromium.org/2416843002/ [4/4] Add browser tests for NavigationPreload. https://codereview.chromium.org/2413643005/ BUG=649558 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). This CL depends on https://codereview.chromium.org/2417793002. FetchEvent.navigationPreload will be implemented in the following steps. [1/4] Introduce FetchEventPreloadHandle to pass the preload response to FetchEvent. https://codereview.chromium.org/2417793002/ [2/4] Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). https://codereview.chromium.org/2410333006/ This CL. [3/4] Implement FetchEvent.navigationPreload. https://codereview.chromium.org/2416843002/ [4/4] Add browser tests for NavigationPreload. https://codereview.chromium.org/2413643005/ BUG=649558 ========== to ========== Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). This CL depends on https://codereview.chromium.org/2417793002. FetchEvent.navigationPreload will be implemented in the following steps. [1/4] Introduce FetchEventPreloadHandle to pass the preload response to FetchEvent. https://codereview.chromium.org/2417793002/ [2/4] Implement ServiceWorkerFetchDispatcher::MaybeStartNavigationPreload(). https://codereview.chromium.org/2410333006/ This CL. [3/4] Implement FetchEvent.navigationPreload. https://codereview.chromium.org/2416843002/ [4/4] Add browser tests for NavigationPreload. https://codereview.chromium.org/2413643005/ BUG=649558 Committed: https://crrev.com/36762af751e2100946cff85db993910e8a402423 Cr-Commit-Position: refs/heads/master@{#426423} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/36762af751e2100946cff85db993910e8a402423 Cr-Commit-Position: refs/heads/master@{#426423} |
