Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2043083002:
Speculatively launch Service Workers on mouse/touch events. [2/5] (Closed)
|
Created:
4 years, 6 months ago by horo Modified:
4 years, 5 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, nhiroki, darin-cc_chromium.org, asvitkine+watch_chromium.org, horo+watch_chromium.org, kinuko+serviceworker, kinuko+watch, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSpeculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ChromeRenderMessageFilter.
NavigationHintSender::maybeSendNavigationHint()
-> sendNavigationHint()
-> WebPrescientNetworking::sendNavigationHint()
-> PrescientNetworkingDispatcher::sendNavigationHint()
-> IPC (NetworkHintsMsg_NavigationHint)
In the browser process, ChromeRenderMessageFilter receives the IPC message.
BUG=616502
Committed: https://crrev.com/133f8c0b69c28928dfbd0d73ea85740da9712791
Cr-Commit-Position: refs/heads/master@{#407399}
Patch Set 1 : with https://codereview.chromium.org/2043863003/#ps60001 #Patch Set 2 : diff from https://codereview.chromium.org/2043863003/#ps60001 #Patch Set 3 : rebase #
Total comments: 4
Patch Set 4 : incorporated falken's comment #
Total comments: 11
Patch Set 5 : incorporated kinuko's comment #Patch Set 6 : use mojo https://codereview.chromium.org/2043753002/#ps220001 #Patch Set 7 : Same as Patch Set 5 #
Total comments: 7
Patch Set 8 : s/RendererNavigationHintSender/RendererNavigationHint/ and add comment #Patch Set 9 : incorporated kinuko's comment #Patch Set 10 : revert BUILD.gn #Messages
Total messages: 83 (48 generated)
Description was changed from ========== 2 ========== to ========== Speculatively launch Service Workers on mouse/touch events. [2/2] [1/2]: https://codereview.chromium.org/2043863003/ [2/2]: This CL. Demo movie: https://drive.google.com/file/d/0B6skYAFVnosEMWdzRm4yMEtIMkk/view?usp=sharing When ChromeRenderMessageFilter receives NavigationHint IPC message, it calls ServiceWorkerContextWrapper::StartServiceWorkerForNavigationHint(). ServiceWorkerContextWrapper will try to start a Service Worker for the document which is linked from the anchor element. If the Service Worker is successfully started, ServiceWorkerVersion::Metrics will measure the precision of the navigation hint. This UMA will be recorded as "ServiceWorker.NavigationHintPrecision" when the worker is stopped. This CL depends on https://codereview.chromium.org/2043863003/ BUG=616502 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Speculatively launch Service Workers on mouse/touch events. [2/2] [1/2]: https://codereview.chromium.org/2043863003/ [2/2]: This CL. Demo movie: https://drive.google.com/file/d/0B6skYAFVnosEMWdzRm4yMEtIMkk/view?usp=sharing When ChromeRenderMessageFilter receives NavigationHint IPC message, it calls ServiceWorkerContextWrapper::StartServiceWorkerForNavigationHint(). ServiceWorkerContextWrapper will try to start a Service Worker for the document which is linked from the anchor element. If the Service Worker is successfully started, ServiceWorkerVersion::Metrics will measure the precision of the navigation hint. This UMA will be recorded as "ServiceWorker.NavigationHintPrecision" when the worker is stopped. This CL depends on https://codereview.chromium.org/2043863003/ BUG=616502 ========== to ========== Speculatively launch Service Workers on mouse/touch events. [2/5] 1/5: Introduce NavigationHintSender. https://codereview.chromium.org/2043863003/ 2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter Thic CL. 3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter https://codereview.chromium.org/2052613003/ 4/5: Measure the precision of the speculative launch of Service Workers for NavigationHints https://codereview.chromium.org/2045153003/ 5/5: Add flags to enable SupeculativeLaunchServiceWorker https://codereview.chromium.org/2053573002/ BUG=616502 ==========
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
BUG=616502
==========
to
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ChromeRenderMessageFilter.
NavigationHintSender::maybeSendNavigationHint()
-> sendNavigationHint()
-> WebPrescientNetworking::sendNavigationHint()
-> PrescientNetworkingDispatcher::sendNavigationHint()
-> RendererPreconnect::NavigationHint()
-> IPC (NetworkHintsMsg_NavigationHint)
In the browser process, ChromeRenderMessageFilter receives the IPC message.
BUG=616502
==========
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #6 (id:220001) has been deleted
Patchset #7 (id:260001) has been deleted
Patchset #6 (id:240001) has been deleted
Patchset #5 (id:200001) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #3 (id:160001) has been deleted
horo@chromium.org changed reviewers: + kouhei@chromium.org
kouhei@ Could you please review this?
horo@chromium.org changed reviewers: + falken@chromium.org
falken@ Could you please review this?
lgtm https://codereview.chromium.org/2043083002/diff/280001/components/network_hin... File components/network_hints/renderer/renderer_navigation_hint_sender.h (right): https://codereview.chromium.org/2043083002/diff/280001/components/network_hin... components/network_hints/renderer/renderer_navigation_hint_sender.h:24: // Submit a navigation hint of mouse/touch events to speculatively launche launch https://codereview.chromium.org/2043083002/diff/280001/components/network_hin... components/network_hints/renderer/renderer_navigation_hint_sender.h:25: // Service Workers. I think SW or even mouse/touch events might not need to be mentioned here, as the navigation hint itself is generic. "Send a hint that a navigation to |url| is likely to happen."?
lgtm % falken's comments https://codereview.chromium.org/2043083002/diff/280001/components/network_hin... File components/network_hints/renderer/renderer_navigation_hint_sender.h (right): https://codereview.chromium.org/2043083002/diff/280001/components/network_hin... components/network_hints/renderer/renderer_navigation_hint_sender.h:25: // Service Workers. On 2016/06/23 04:10:29, falken wrote: > I think SW or even mouse/touch events might not need to be mentioned here, as > the navigation hint itself is generic. "Send a hint that a navigation to |url| > is likely to happen."? +1
Thank you https://codereview.chromium.org/2043083002/diff/280001/components/network_hin... File components/network_hints/renderer/renderer_navigation_hint_sender.h (right): https://codereview.chromium.org/2043083002/diff/280001/components/network_hin... components/network_hints/renderer/renderer_navigation_hint_sender.h:25: // Service Workers. On 2016/06/23 04:10:29, falken wrote: > I think SW or even mouse/touch events might not need to be mentioned here, as > the navigation hint itself is generic. "Send a hint that a navigation to |url| > is likely to happen."? Done.
horo@chromium.org changed reviewers: + kinuko@chromium.org
kinuko@ Could you please review?
/cc +csharrison for routing this via components/network_hints -> chrome_render_message_filter (the path we're deprecating in the other issue). https://codereview.chromium.org/2043083002/diff/300001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.h (right): https://codereview.chromium.org/2043083002/diff/300001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.h:58: void OnNavigationHint(const GURL& url, blink::WebNavigationHintType type); By the way we started to move these network hints code outside chrome into content + mojo (See crbug.com/610750). In our case I feel it's even less clear why we need to go through chrome... we probably don't need to? https://codereview.chromium.org/2043083002/diff/300001/components/network_hin... File components/network_hints/renderer/prescient_networking_dispatcher.h (right): https://codereview.chromium.org/2043083002/diff/300001/components/network_hin... components/network_hints/renderer/prescient_networking_dispatcher.h:31: blink::WebNavigationHintType type) override; This class mostly has requests to the network stack, I'm not fully sure if this fits in this class perfectly. Could we ask network_hints component owners first? https://codereview.chromium.org/2043083002/diff/300001/components/network_hin... File components/network_hints/renderer/renderer_navigation_hint_sender.h (right): https://codereview.chromium.org/2043083002/diff/300001/components/network_hin... components/network_hints/renderer/renderer_navigation_hint_sender.h:29: }; // class RendererNavigationHintSender nit: this end class comment probably not necessary? https://codereview.chromium.org/2043083002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043083002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:111: blink::sendNavigationHint(m_anchorElement->href(), type); blink:: doesn't seem necessary https://codereview.chromium.org/2043083002/diff/300001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebNavigationHintType.h (right): https://codereview.chromium.org/2043083002/diff/300001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebNavigationHintType.h:11: Unknown, Do we actually use this Unknown type? https://codereview.chromium.org/2043083002/diff/300001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebPrescientNetworking.h (right): https://codereview.chromium.org/2043083002/diff/300001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebPrescientNetworking.h:52: virtual void sendNavigationHint(const WebURL& url, WebNavigationHintType) {} Please have a comment for this
https://codereview.chromium.org/2043083002/diff/300001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.h (right): https://codereview.chromium.org/2043083002/diff/300001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.h:58: void OnNavigationHint(const GURL& url, blink::WebNavigationHintType type); On 2016/06/24 07:40:47, kinuko wrote: > By the way we started to move these network hints code outside chrome into > content + mojo (See crbug.com/610750). In our case I feel it's even less clear > why we need to go through chrome... we probably don't need to? I just wanted to align with DnsPrefetch and Preconnect. But this completely conflicts with crbug.com/610750. csharrison@ Do you have any idea about when crbug.com/610750 will be fixed? https://codereview.chromium.org/2043083002/diff/300001/components/network_hin... File components/network_hints/renderer/renderer_navigation_hint_sender.h (right): https://codereview.chromium.org/2043083002/diff/300001/components/network_hin... components/network_hints/renderer/renderer_navigation_hint_sender.h:29: }; // class RendererNavigationHintSender On 2016/06/24 07:40:47, kinuko wrote: > nit: this end class comment probably not necessary? Done. https://codereview.chromium.org/2043083002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2043083002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:111: blink::sendNavigationHint(m_anchorElement->href(), type); On 2016/06/24 07:40:48, kinuko wrote: > blink:: doesn't seem necessary Done. https://codereview.chromium.org/2043083002/diff/300001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebNavigationHintType.h (right): https://codereview.chromium.org/2043083002/diff/300001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebNavigationHintType.h:11: Unknown, On 2016/06/24 07:40:48, kinuko wrote: > Do we actually use this Unknown type? Removed https://codereview.chromium.org/2043083002/diff/300001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebPrescientNetworking.h (right): https://codereview.chromium.org/2043083002/diff/300001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebPrescientNetworking.h:52: virtual void sendNavigationHint(const WebURL& url, WebNavigationHintType) {} On 2016/06/24 07:40:48, kinuko wrote: > Please have a comment for this Done.
horo@chromium.org changed reviewers: + juliatuttle@chromium.org
juliatuttle@chromium.org Could you please review components/network_hints/*?
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
I am planning on completely removing the network_hints component. The new code will go through mojo here: https://codereview.chromium.org/2043753002/ I'm fine with adding this as-is due to its speculative nature (2043753002 doesn't deal with link hover / a tag scanning for dns yet), but could you try to have the actual speculative SW generation in content/browser/loader/resource_hints_impl, so that mojo code can eventually use it? Possibly, I named that class a bit too specific and should have stuck with network hints :)
Description was changed from
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ChromeRenderMessageFilter.
NavigationHintSender::maybeSendNavigationHint()
-> sendNavigationHint()
-> WebPrescientNetworking::sendNavigationHint()
-> PrescientNetworkingDispatcher::sendNavigationHint()
-> RendererPreconnect::NavigationHint()
-> IPC (NetworkHintsMsg_NavigationHint)
In the browser process, ChromeRenderMessageFilter receives the IPC message.
BUG=616502
==========
to
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ResourceHintsHandlerImpl
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ResourceHintsHandlerImpl
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ResourceHintsHandlerImpl.
NavigationHintSender::maybeSendNavigationHint()
-> blink::sendNavigationHint()
-> Mojo IPC (SendNavigationHint)
In the browser process, ResourceHintsHandlerImpl receives the mojo IPC message.
BUG=616502
==========
On 2016/06/24 12:24:55, csharrison wrote: > I am planning on completely removing the network_hints component. The new code > will go through mojo here: > https://codereview.chromium.org/2043753002/ > > I'm fine with adding this as-is due to its speculative nature (2043753002 > doesn't deal with link hover / a tag scanning for dns yet), but could you try to > have the actual speculative SW generation in > content/browser/loader/resource_hints_impl, so that mojo code can eventually use > it? > > Possibly, I named that class a bit too specific and should have stuck with > network hints :) I uploaded PatchSet 6 which use mojo IPC. I will wait for https://codereview.chromium.org/2043753002/ to be submitted. ResourceHintsHandlerImpl::SendNavigationHint() will call StartServiceWorkerForNavigationHint in the next cl (https://codereview.chromium.org/2052613003/).
csharrison@ What is the status of your patch (https://codereview.chromium.org/2043753002/)? If you are not working on it now, I'd like to add NavigationHint IPC message in network_hints_messages.h before your patch.
Feel free to land this. The CL hit a road block unfortunately that we're trying to sort out.
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.
I uploaded Patch Set 7 which is same as Patch Set 5. kinuko@ Could you please review Patch Set 7? juliatuttle@ Could you please review components/network_hints/*?
Description was changed from
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ResourceHintsHandlerImpl
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ResourceHintsHandlerImpl
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ResourceHintsHandlerImpl.
NavigationHintSender::maybeSendNavigationHint()
-> blink::sendNavigationHint()
-> Mojo IPC (SendNavigationHint)
In the browser process, ResourceHintsHandlerImpl receives the mojo IPC message.
BUG=616502
==========
to
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ResourceHintsHandlerImpl
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ResourceHintsHandlerImpl
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ChromeRenderMessageFilter.
NavigationHintSender::maybeSendNavigationHint()
-> sendNavigationHint()
-> WebPrescientNetworking::sendNavigationHint()
-> PrescientNetworkingDispatcher::sendNavigationHint()
-> RendererPreconnect::NavigationHint()
-> IPC (NetworkHintsMsg_NavigationHint)
In the browser process, ChromeRenderMessageFilter receives the IPC message.
BUG=616502
==========
Description was changed from
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ResourceHintsHandlerImpl
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ResourceHintsHandlerImpl
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ChromeRenderMessageFilter.
NavigationHintSender::maybeSendNavigationHint()
-> sendNavigationHint()
-> WebPrescientNetworking::sendNavigationHint()
-> PrescientNetworkingDispatcher::sendNavigationHint()
-> RendererPreconnect::NavigationHint()
-> IPC (NetworkHintsMsg_NavigationHint)
In the browser process, ChromeRenderMessageFilter receives the IPC message.
BUG=616502
==========
to
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ChromeRenderMessageFilter.
NavigationHintSender::maybeSendNavigationHint()
-> sendNavigationHint()
-> WebPrescientNetworking::sendNavigationHint()
-> PrescientNetworkingDispatcher::sendNavigationHint()
-> RendererPreconnect::NavigationHint()
-> IPC (NetworkHintsMsg_NavigationHint)
In the browser process, ChromeRenderMessageFilter receives the IPC message.
BUG=616502
==========
components/network_hints lgtm with one nit. What's the deal with this and the CL to mojoify network_hints? It looks like you reworked this to use mojo and then backed it out? https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... File components/network_hints/renderer/renderer_navigation_hint_sender.h (right): https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... components/network_hints/renderer/renderer_navigation_hint_sender.h:19: class RendererNavigationHintSender { I would drop the "Sender" from the name here, for parallelism with RendererDnsPrefetch and RendererPreconnect.
On 2016/07/18 14:44:37, Julia Tuttle wrote: > components/network_hints lgtm with one nit. > > What's the deal with this and the CL to mojoify network_hints? It looks like you > reworked this to use mojo and then backed it out? > > https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... > File components/network_hints/renderer/renderer_navigation_hint_sender.h > (right): > > https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... > components/network_hints/renderer/renderer_navigation_hint_sender.h:19: class > RendererNavigationHintSender { > I would drop the "Sender" from the name here, for parallelism with > RendererDnsPrefetch and RendererPreconnect. Still planning on landing the mojo change, but we need to figure out a few things wrt ResourceContext lifetimes. There's no clear consensus on what should be done and I haven't had the cycles to do an analysis of all the options.
https://codereview.chromium.org/2043083002/diff/360001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/2043083002/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.cc:135: // TODO(horo): Implement this. I really don't think this should go through //chrome, could we have a TODO comment here that having this in chrome is not really desirable/necessary and we should consider moving this to //content? https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... File components/network_hints/renderer/prescient_networking_dispatcher.cc (right): https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... components/network_hints/renderer/prescient_networking_dispatcher.cc:36: navigation_hint_sender_.sendNavigationHint(url, type); Why do we need a separate navigation hint sender object here? All we need is to send an IPC message, right?
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/2043083002/diff/360001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/2043083002/diff/360001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_render_message_filter.cc:135: // TODO(horo): Implement this. On 2016/07/18 15:24:45, kinuko wrote: > I really don't think this should go through //chrome, could we have a TODO > comment here that having this in chrome is not really desirable/necessary and we > should consider moving this to //content? Done. https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... File components/network_hints/renderer/prescient_networking_dispatcher.cc (right): https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... components/network_hints/renderer/prescient_networking_dispatcher.cc:36: navigation_hint_sender_.sendNavigationHint(url, type); On 2016/07/18 15:24:45, kinuko wrote: > Why do we need a separate navigation hint sender object here? All we need is to > send an IPC message, right? I just wanted to do the same thing as RendererPreconnect. https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... File components/network_hints/renderer/renderer_navigation_hint_sender.h (right): https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... components/network_hints/renderer/renderer_navigation_hint_sender.h:19: class RendererNavigationHintSender { On 2016/07/18 14:44:37, Julia Tuttle wrote: > I would drop the "Sender" from the name here, for parallelism with > RendererDnsPrefetch and RendererPreconnect. Done.
https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... File components/network_hints/renderer/prescient_networking_dispatcher.cc (right): https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... components/network_hints/renderer/prescient_networking_dispatcher.cc:36: navigation_hint_sender_.sendNavigationHint(url, type); On 2016/07/19 02:09:43, horo wrote: > On 2016/07/18 15:24:45, kinuko wrote: > > Why do we need a separate navigation hint sender object here? All we need is > to > > send an IPC message, right? > > I just wanted to do the same thing as RendererPreconnect. They have things to do there... what value does it bring to have a separate object here in our case?
On 2016/07/19 02:17:08, kinuko wrote: > https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... > File components/network_hints/renderer/prescient_networking_dispatcher.cc > (right): > > https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... > components/network_hints/renderer/prescient_networking_dispatcher.cc:36: > navigation_hint_sender_.sendNavigationHint(url, type); > On 2016/07/19 02:09:43, horo wrote: > > On 2016/07/18 15:24:45, kinuko wrote: > > > Why do we need a separate navigation hint sender object here? All we need > is > > to > > > send an IPC message, right? > > > > I just wanted to do the same thing as RendererPreconnect. > > They have things to do there... what value does it bring to have a separate > object here in our case? Humm.. I think RendererPreconnect is also just sending IPC messages. https://cs.chromium.org/chromium/src/components/network_hints/renderer/render... Shall I move the both "RenderThread::Get()->Send()" to prescient_networking_dispatcher.cc?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/19 03:05:11, horo wrote: > On 2016/07/19 02:17:08, kinuko wrote: > > > https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... > > File components/network_hints/renderer/prescient_networking_dispatcher.cc > > (right): > > > > > https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... > > components/network_hints/renderer/prescient_networking_dispatcher.cc:36: > > navigation_hint_sender_.sendNavigationHint(url, type); > > On 2016/07/19 02:09:43, horo wrote: > > > On 2016/07/18 15:24:45, kinuko wrote: > > > > Why do we need a separate navigation hint sender object here? All we need > > is > > > to > > > > send an IPC message, right? > > > > > > I just wanted to do the same thing as RendererPreconnect. > > > > They have things to do there... what value does it bring to have a separate > > object here in our case? > > Humm.. > I think RendererPreconnect is also just sending IPC messages. > https://cs.chromium.org/chromium/src/components/network_hints/renderer/render... > > Shall I move the both "RenderThread::Get()->Send()" to > prescient_networking_dispatcher.cc? Aha. Looks like we used to have more things to do in renderer_preconnect.cc but that was removed at some point (https://codereview.chromium.org/922533003/). I'd just replace our one with RenderThread::Get()->Send() for our one for now, and create a small cleanup patch for making the same change.
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/07/19 03:47:57, kinuko wrote: > On 2016/07/19 03:05:11, horo wrote: > > On 2016/07/19 02:17:08, kinuko wrote: > > > > > > https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... > > > File components/network_hints/renderer/prescient_networking_dispatcher.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... > > > components/network_hints/renderer/prescient_networking_dispatcher.cc:36: > > > navigation_hint_sender_.sendNavigationHint(url, type); > > > On 2016/07/19 02:09:43, horo wrote: > > > > On 2016/07/18 15:24:45, kinuko wrote: > > > > > Why do we need a separate navigation hint sender object here? All we > need > > > is > > > > to > > > > > send an IPC message, right? > > > > > > > > I just wanted to do the same thing as RendererPreconnect. > > > > > > They have things to do there... what value does it bring to have a separate > > > object here in our case? > > > > Humm.. > > I think RendererPreconnect is also just sending IPC messages. > > > https://cs.chromium.org/chromium/src/components/network_hints/renderer/render... > > > > Shall I move the both "RenderThread::Get()->Send()" to > > prescient_networking_dispatcher.cc? > > Aha. Looks like we used to have more things to do in renderer_preconnect.cc but > that was removed at some point (https://codereview.chromium.org/922533003/). > I'd just replace our one with RenderThread::Get()->Send() for our one for now, > and create a small cleanup patch for making the same change. doen
On 2016/07/19 03:47:57, kinuko wrote: > On 2016/07/19 03:05:11, horo wrote: > > On 2016/07/19 02:17:08, kinuko wrote: > > > > > > https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... > > > File components/network_hints/renderer/prescient_networking_dispatcher.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2043083002/diff/360001/components/network_hin... > > > components/network_hints/renderer/prescient_networking_dispatcher.cc:36: > > > navigation_hint_sender_.sendNavigationHint(url, type); > > > On 2016/07/19 02:09:43, horo wrote: > > > > On 2016/07/18 15:24:45, kinuko wrote: > > > > > Why do we need a separate navigation hint sender object here? All we > need > > > is > > > > to > > > > > send an IPC message, right? > > > > > > > > I just wanted to do the same thing as RendererPreconnect. > > > > > > They have things to do there... what value does it bring to have a separate > > > object here in our case? > > > > Humm.. > > I think RendererPreconnect is also just sending IPC messages. > > > https://cs.chromium.org/chromium/src/components/network_hints/renderer/render... > > > > Shall I move the both "RenderThread::Get()->Send()" to > > prescient_networking_dispatcher.cc? > > Aha. Looks like we used to have more things to do in renderer_preconnect.cc but > that was removed at some point (https://codereview.chromium.org/922533003/). > I'd just replace our one with RenderThread::Get()->Send() for our one for now, > and create a small cleanup patch for making the same change. done
lgtm
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 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...
horo@chromium.org changed reviewers: + thestig@chromium.org
thestig@ Could you please review chrome/browser/renderer_host/chrome_render_message_filter.*?
On 2016/07/19 04:10:12, horo wrote: > thestig@ > Could you please review > chrome/browser/renderer_host/chrome_render_message_filter.*? c/b/renderer_host lgtm given charrison's comments in https://codereview.chromium.org/2043083002/#msg34
Description was changed from
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ChromeRenderMessageFilter.
NavigationHintSender::maybeSendNavigationHint()
-> sendNavigationHint()
-> WebPrescientNetworking::sendNavigationHint()
-> PrescientNetworkingDispatcher::sendNavigationHint()
-> RendererPreconnect::NavigationHint()
-> IPC (NetworkHintsMsg_NavigationHint)
In the browser process, ChromeRenderMessageFilter receives the IPC message.
BUG=616502
==========
to
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ChromeRenderMessageFilter.
NavigationHintSender::maybeSendNavigationHint()
-> sendNavigationHint()
-> WebPrescientNetworking::sendNavigationHint()
-> PrescientNetworkingDispatcher::sendNavigationHint()
-> IPC (NetworkHintsMsg_NavigationHint)
In the browser process, ChromeRenderMessageFilter receives the IPC message.
BUG=616502
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, kouhei@chromium.org, juliatuttle@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2043083002/#ps420001 (title: "revert BUILD.gn")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
horo@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@ Could you please review components/network_hints/common/network_hints_messages.h?
The IPC changes in this CL and https://codereview.chromium.org/2052613003/ LGTM. However, it would be better to combine the CLs where we add the IPC and where we implement the actual IPC, since the implementation is part of the security review. For the followup CL, I would suggest using OverrideThreadForMessage to simplify some of the code: that way, we don't need to post task to the UI thread and then post task back: we can just have the message filter forward the message to the UI thread for handling, and then call back to the IO thread when the RenderProcessHostImpl check is done.
On 2016/07/22 04:19:16, dcheng wrote: > The IPC changes in this CL and https://codereview.chromium.org/2052613003/ LGTM. > However, it would be better to combine the CLs where we add the IPC and where we > implement the actual IPC, since the implementation is part of the security > review. > > For the followup CL, I would suggest using OverrideThreadForMessage to simplify > some of the code: that way, we don't need to post task to the UI thread and then > post task back: we can just have the message filter forward the message to the > UI thread for handling, and then call back to the IO thread when the > RenderProcessHostImpl check is done.
On 2016/07/22 04:19:16, dcheng wrote: > The IPC changes in this CL and https://codereview.chromium.org/2052613003/ LGTM. > However, it would be better to combine the CLs where we add the IPC and where we > implement the actual IPC, since the implementation is part of the security > review. > > For the followup CL, I would suggest using OverrideThreadForMessage to simplify > some of the code: that way, we don't need to post task to the UI thread and then > post task back: we can just have the message filter forward the message to the > UI thread for handling, and then call back to the IO thread when the > RenderProcessHostImpl check is done. Thank you. I will use OverrideThreadForMessage in the next CL.
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
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ChromeRenderMessageFilter.
NavigationHintSender::maybeSendNavigationHint()
-> sendNavigationHint()
-> WebPrescientNetworking::sendNavigationHint()
-> PrescientNetworkingDispatcher::sendNavigationHint()
-> IPC (NetworkHintsMsg_NavigationHint)
In the browser process, ChromeRenderMessageFilter receives the IPC message.
BUG=616502
==========
to
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ChromeRenderMessageFilter.
NavigationHintSender::maybeSendNavigationHint()
-> sendNavigationHint()
-> WebPrescientNetworking::sendNavigationHint()
-> PrescientNetworkingDispatcher::sendNavigationHint()
-> IPC (NetworkHintsMsg_NavigationHint)
In the browser process, ChromeRenderMessageFilter receives the IPC message.
BUG=616502
==========
Message was sent while issue was closed.
Committed patchset #10 (id:420001)
Message was sent while issue was closed.
Description was changed from
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ChromeRenderMessageFilter.
NavigationHintSender::maybeSendNavigationHint()
-> sendNavigationHint()
-> WebPrescientNetworking::sendNavigationHint()
-> PrescientNetworkingDispatcher::sendNavigationHint()
-> IPC (NetworkHintsMsg_NavigationHint)
In the browser process, ChromeRenderMessageFilter receives the IPC message.
BUG=616502
==========
to
==========
Speculatively launch Service Workers on mouse/touch events. [2/5]
1/5: Introduce NavigationHintSender.
https://codereview.chromium.org/2043863003/
2/5: Pipe NavigationHints from NavigationHintSender to ChromeRenderMessageFilter
Thic CL.
3/5: Call StartServiceWorkerForNavigationHint() from ChromeRenderMessageFilter
https://codereview.chromium.org/2052613003/
4/5: Measure the precision of the speculative launch of Service Workers for
NavigationHints
https://codereview.chromium.org/2045153003/
5/5: Add flags to enable SupeculativeLaunchServiceWorker
https://codereview.chromium.org/2053573002/
NavigationHints are sent from NavigationHintSender to ChromeRenderMessageFilter.
NavigationHintSender::maybeSendNavigationHint()
-> sendNavigationHint()
-> WebPrescientNetworking::sendNavigationHint()
-> PrescientNetworkingDispatcher::sendNavigationHint()
-> IPC (NetworkHintsMsg_NavigationHint)
In the browser process, ChromeRenderMessageFilter receives the IPC message.
BUG=616502
Committed: https://crrev.com/133f8c0b69c28928dfbd0d73ea85740da9712791
Cr-Commit-Position: refs/heads/master@{#407399}
==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/133f8c0b69c28928dfbd0d73ea85740da9712791 Cr-Commit-Position: refs/heads/master@{#407399} |
