Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14746002:
Add WebPrescientNetworking to trigger preconnect from Blink. (Closed)
|
Created:
7 years, 7 months ago by kouhei (in TOK) Modified:
7 years, 7 months ago CC:
blink-reviews, jamesr, jeez, abarth_chromum.org, haraken, cbentzel, tburkard Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd a new interface WebPrescientNetworking to trigger preconnect from Blink.
The preconnect from Blink is always issued with motivation information WebPreconnectMotivation, so we can evaluate precision/coverage of the triggers.
This API is to be triggered from {mouse,gesture}-events on HTMLAnchorElement.
BUG=235361
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150344
Patch Set 1 #Patch Set 2 : revised motivation list #Patch Set 3 : add preconnect if to WebPrerenderingSupport #Patch Set 4 : minimize cl #
Total comments: 3
Patch Set 5 : change comment #Patch Set 6 : fix coding style #
Total comments: 6
Patch Set 7 : 'use static_cast / no pure virtual #
Total comments: 5
Patch Set 8 : nits #
Total comments: 5
Patch Set 9 : use WebPrescientNetworking instead of WebPrerenderingSupport #
Total comments: 16
Patch Set 10 : access WebPrescientNetworking via Platform #Patch Set 11 : remove core.gypi from CL #Patch Set 12 : fix comment #
Total comments: 4
Patch Set 13 : add missing decl #Patch Set 14 : add virtual d-tor #
Messages
Total messages: 28 (0 generated)
Could you review this patch?
+CCs
The below patch covers the implementation of the platform API added in this patch: https://codereview.chromium.org/14746002/
Oops. I meant: https://codereview.chromium.org/14749005/
I don't think this should be in the platform API, which provides functionality to help Blink implementation. Can you move this into a something in loader clients? 'motivation' sounds too emotional. I prefer 'trigger.'
On 2013/05/01 23:07:28, tkent wrote: > I don't think this should be in the platform API, which provides functionality > to help Blink implementation. Can you move this into a something in loader > clients? Thank you for your comments. I am currently experimenting if this interface can be merged into WebPrerendererClient, instead of platform API. > 'motivation' sounds too emotional. I prefer 'trigger.' 'motivation' is already used in existing preconnect and prerender implementation. I tried to be consistent with them.
Updated patch. Could you review? The patch now uses WebPrerenderingSupport instead of platform API.
Gavin knows the WebPrerenderingSupport bit the best, adding him to review list.
I don't own Source/Platform/. https://codereview.chromium.org/14746002/diff/13001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebPrerenderingSupport.h (right): https://codereview.chromium.org/14746002/diff/13001/Source/Platform/chromium/... Source/Platform/chromium/public/WebPrerenderingSupport.h:48: // preconnect to the host providing resource specified by given url. It's hard to understand this comment. What does this function do concretely? When should we call this? What is 'host'? nit: a comment should start a capital letter. https://codereview.chromium.org/14746002/diff/13001/Source/core/platform/netw... File Source/core/platform/network/Preconnect.cpp (right): https://codereview.chromium.org/14746002/diff/13001/Source/core/platform/netw... Source/core/platform/network/Preconnect.cpp:32: #include "Preconnect.h" should be core/platform/network/Preconnect.h
Thank you for comments! https://chromiumcodereview.appspot.com/14746002/diff/13001/Source/core/platfo... File Source/core/platform/network/Preconnect.cpp (right): https://chromiumcodereview.appspot.com/14746002/diff/13001/Source/core/platfo... Source/core/platform/network/Preconnect.cpp:32: #include "Preconnect.h" On 2013/05/08 23:12:47, tkent wrote: > should be core/platform/network/Preconnect.h Done. https://chromiumcodereview.appspot.com/14746002/diff/20002/Source/Platform/ch... File Source/Platform/chromium/public/WebPrerenderingSupport.h (right): https://chromiumcodereview.appspot.com/14746002/diff/20002/Source/Platform/ch... Source/Platform/chromium/public/WebPrerenderingSupport.h:49: // session initialization latency to the server providing the page resource. Changed comment. Does it make sense now?
https://chromiumcodereview.appspot.com/14746002/diff/20002/Source/Platform/ch... File Source/Platform/chromium/public/WebPrerenderingSupport.h (right): https://chromiumcodereview.appspot.com/14746002/diff/20002/Source/Platform/ch... Source/Platform/chromium/public/WebPrerenderingSupport.h:49: // session initialization latency to the server providing the page resource. On 2013/05/09 01:08:34, Kouhei_UENO wrote: > Changed comment. Does it make sense now? Yeah, it makes sense. https://chromiumcodereview.appspot.com/14746002/diff/20002/Source/core/platfo... File Source/core/platform/network/Preconnect.cpp (right): https://chromiumcodereview.appspot.com/14746002/diff/20002/Source/core/platfo... Source/core/platform/network/Preconnect.cpp:44: case PreconnectMotivationLinkMouseDown: For such 1:1 mapping, we usually just do static_cast<>(), and add tests to Source/WebKit/chromium/src/AssertMatchingEnums.cpp. https://chromiumcodereview.appspot.com/14746002/diff/20002/Source/core/platfo... File Source/core/platform/network/Preconnect.h (right): https://chromiumcodereview.appspot.com/14746002/diff/20002/Source/core/platfo... Source/core/platform/network/Preconnect.h:34: #include <wtf/Forward.h> should use "" for WTF too.
Thanks! Updated patch. https://codereview.chromium.org/14746002/diff/20002/Source/core/platform/netw... File Source/core/platform/network/Preconnect.cpp (right): https://codereview.chromium.org/14746002/diff/20002/Source/core/platform/netw... Source/core/platform/network/Preconnect.cpp:44: case PreconnectMotivationLinkMouseDown: On 2013/05/09 02:33:55, tkent wrote: > For such 1:1 mapping, we usually just do static_cast<>(), and add tests to > Source/WebKit/chromium/src/AssertMatchingEnums.cpp. Done. https://codereview.chromium.org/14746002/diff/20002/Source/core/platform/netw... File Source/core/platform/network/Preconnect.h (right): https://codereview.chromium.org/14746002/diff/20002/Source/core/platform/netw... Source/core/platform/network/Preconnect.h:34: #include <wtf/Forward.h> On 2013/05/09 02:33:55, tkent wrote: > should use "" for WTF too. Done. https://codereview.chromium.org/14746002/diff/27001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebPrerenderingSupport.h (right): https://codereview.chromium.org/14746002/diff/27001/Source/Platform/chromium/... Source/Platform/chromium/public/WebPrerenderingSupport.h:51: WebPreconnectMotivation motivation) {} This is to enable Chromium to be compiled before the impl patch land. I am not sure if this is the right way to do this.
lgtm for files owned by me. https://codereview.chromium.org/14746002/diff/27001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebPrerenderingSupport.h (right): https://codereview.chromium.org/14746002/diff/27001/Source/Platform/chromium/... Source/Platform/chromium/public/WebPrerenderingSupport.h:51: WebPreconnectMotivation motivation) {} On 2013/05/09 03:19:29, Kouhei_UENO wrote: > This is to enable Chromium to be compiled before the impl patch land. I am not > sure if this is the right way to do this. It's a common way. However you should put "{ }" (space between braces) for consistency. https://codereview.chromium.org/14746002/diff/27001/Source/core/platform/netw... File Source/core/platform/network/Preconnect.h (right): https://codereview.chromium.org/14746002/diff/27001/Source/core/platform/netw... Source/core/platform/network/Preconnect.h:34: #include "wtf/Forward.h" Please sort includes.
Updated patch for nits. Thanks! https://codereview.chromium.org/14746002/diff/27001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebPrerenderingSupport.h (right): https://codereview.chromium.org/14746002/diff/27001/Source/Platform/chromium/... Source/Platform/chromium/public/WebPrerenderingSupport.h:51: WebPreconnectMotivation motivation) {} On 2013/05/09 03:31:16, tkent wrote: > On 2013/05/09 03:19:29, Kouhei_UENO wrote: > > This is to enable Chromium to be compiled before the impl patch land. I am not > > sure if this is the right way to do this. > > It's a common way. > However you should put "{ }" (space between braces) for consistency. > > Done. https://codereview.chromium.org/14746002/diff/27001/Source/core/platform/netw... File Source/core/platform/network/Preconnect.h (right): https://codereview.chromium.org/14746002/diff/27001/Source/core/platform/netw... Source/core/platform/network/Preconnect.h:34: #include "wtf/Forward.h" On 2013/05/09 03:31:16, tkent wrote: > Please sort includes. Done.
Nice. Should we also add a rel type to the link element (in a later CL?)
abarth: Could you review this patch? In specific, Source/Platform/chromium/public/*?
> Should we also add a rel type to the link element (in a later CL?) I don't think a <link> rel attr exist for preconnecting. Could you elaborate on this?
On 2013/05/10 00:43:39, Kouhei_UENO wrote: > > Should we also add a rel type to the link element (in a later CL?) > I don't think a <link> rel attr exist for preconnecting. Could you elaborate on > this? It would be nice to expose this functionality similarly to how we expose dns prefetch. It could be added to loader/LinkLoader.* pretty easily.
https://codereview.chromium.org/14746002/diff/33001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebPreconnectMotivation.h (right): https://codereview.chromium.org/14746002/diff/33001/Source/Platform/chromium/... Source/Platform/chromium/public/WebPreconnectMotivation.h:40: WebPreconnectMotivationLinkTapDown, Technically this is a layering violation. The Platform layer doesn't have any concept of a "link", "mouse", or "tap". What is this used for? Can we use names that are more descriptive of what the platform should do with this information? https://codereview.chromium.org/14746002/diff/33001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebPrerenderingSupport.h (right): https://codereview.chromium.org/14746002/diff/33001/Source/Platform/chromium/... Source/Platform/chromium/public/WebPrerenderingSupport.h:51: WebPreconnectMotivation motivation) { } This API shouldn't be coupled with WebPrerenderingSupport. It should be directly on Platform, similar to: WebKit::Platform::current()->prefetchHostName(hostname); https://codereview.chromium.org/14746002/diff/33001/Source/core/platform/netw... File Source/core/platform/network/Preconnect.h (right): https://codereview.chromium.org/14746002/diff/33001/Source/core/platform/netw... Source/core/platform/network/Preconnect.h:46: void preconnect(const KURL& url, PreconnectMotivation motivation); Once you move preconnect directly to Platform.h, you can delete this file and it's CPP. Core code can call into WebKit::Platform()->preconnect directly.
https://codereview.chromium.org/14746002/diff/33001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebPreconnectMotivation.h (right): https://codereview.chromium.org/14746002/diff/33001/Source/Platform/chromium/... Source/Platform/chromium/public/WebPreconnectMotivation.h:40: WebPreconnectMotivationLinkTapDown, On 2013/05/10 16:57:25, abarth wrote: > Technically this is a layering violation. The Platform layer doesn't have any > concept of a "link", "mouse", or "tap". > > What is this used for? Can we use names that are more descriptive of what the > platform should do with this information? I admit this is some layering violation. However, I would like to have this temporarily for my purpose. What I would like to do is a Finch trial to compare effect of preconnects from different triggers. To do that, I would like to enable preconnect from each trigger one at a time. However, I couldn't find an interface for FieldTrial class from Blink, so I am planning to pass this information to predictor, and proceed with preconnect if the user is in an experiment group. https://codereview.chromium.org/14746002/diff/33001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebPrerenderingSupport.h (right): https://codereview.chromium.org/14746002/diff/33001/Source/Platform/chromium/... Source/Platform/chromium/public/WebPrerenderingSupport.h:51: WebPreconnectMotivation motivation) { } On 2013/05/10 16:57:25, abarth wrote: > This API shouldn't be coupled with WebPrerenderingSupport. It should be > directly on Platform, similar to: > > WebKit::Platform::current()->prefetchHostName(hostname); Actually, this patch was first created as an addition to platform API, but I heard some concerns: https://codereview.chromium.org/14749005/diff/1/content/renderer/renderer_web... https://codereview.chromium.org/14746002/#msg5 I think it is best to have prefetching APIs (prefetch DNS, preconnect, prerendering) to be in same place. I was thinking about having all these in PrerenderingSupport, then renaming it adequately.
On 2013/05/13 01:31:47, Kouhei_UENO wrote: > https://codereview.chromium.org/14746002/diff/33001/Source/Platform/chromium/... > File Source/Platform/chromium/public/WebPreconnectMotivation.h (right): > > https://codereview.chromium.org/14746002/diff/33001/Source/Platform/chromium/... > Source/Platform/chromium/public/WebPreconnectMotivation.h:40: > WebPreconnectMotivationLinkTapDown, > On 2013/05/10 16:57:25, abarth wrote: > > Technically this is a layering violation. The Platform layer doesn't have any > > concept of a "link", "mouse", or "tap". > > > > What is this used for? Can we use names that are more descriptive of what the > > platform should do with this information? > I admit this is some layering violation. However, I would like to have this > temporarily for my purpose. > > What I would like to do is a Finch trial to compare effect of preconnects from > different triggers. To do that, I would like to enable preconnect from each > trigger one at a time. However, I couldn't find an interface for FieldTrial > class from Blink, so I am planning to pass this information to predictor, and > proceed with preconnect if the user is in an experiment group. Would be willing to add a comment explaining this reasoning in the header? That will help future people reading this code understand why the layering violation exists (and hopefully remind us to get rid of it at some point). > https://codereview.chromium.org/14746002/diff/33001/Source/Platform/chromium/... > File Source/Platform/chromium/public/WebPrerenderingSupport.h (right): > > https://codereview.chromium.org/14746002/diff/33001/Source/Platform/chromium/... > Source/Platform/chromium/public/WebPrerenderingSupport.h:51: > WebPreconnectMotivation motivation) { } > On 2013/05/10 16:57:25, abarth wrote: > > This API shouldn't be coupled with WebPrerenderingSupport. It should be > > directly on Platform, similar to: > > > > WebKit::Platform::current()->prefetchHostName(hostname); > > Actually, this patch was first created as an addition to platform API, but I > heard some concerns: > https://codereview.chromium.org/14749005/diff/1/content/renderer/renderer_web... > https://codereview.chromium.org/14746002/#msg5 > > I think it is best to have prefetching APIs (prefetch DNS, preconnect, > prerendering) to be in same place. I was thinking about having all these in > PrerenderingSupport, then renaming it adequately. As usual, jam is right. I agree that the best approach here is to create a new interface to hold DNSPrefetch and preconnect, but I'd keep it separate from WebPrerenderingSupport. Perhaps WebPrescientNetworking?
Updated patch to use a new interface WebPrescientNetworking. Its implementation is on https://codereview.chromium.org/14749005/ abarth: Would you take a look? > Would be willing to add a comment explaining this reasoning in the header? That > will help future people reading this code understand why the layering violation > exists (and hopefully remind us to get rid of it at some point). Done. > As usual, jam is right. I agree that the best approach here is to create a new > interface to hold DNSPrefetch and preconnect, but I'd keep it separate from > WebPrerenderingSupport. Perhaps WebPrescientNetworking? After this patch, I would be happy to refactor DNSPrefetch to use WebprescientNetworking.
https://codereview.chromium.org/14746002/diff/43001/Source/Platform/chromium/... File Source/Platform/chromium/public/WebPrescientNetworking.h (right): https://codereview.chromium.org/14746002/diff/43001/Source/Platform/chromium/... Source/Platform/chromium/public/WebPrescientNetworking.h:40: // FIXME (kouhei): Passing preconnect motivation to net is layering vioration. " (kouhei)" <-- you should delete this part. We don't put user names in FIXME comments in Blink. net -> "the platform". Blink has no knowledge that a "net" module exists. https://codereview.chromium.org/14746002/diff/43001/Source/Platform/chromium/... Source/Platform/chromium/public/WebPrescientNetworking.h:55: WEBKIT_EXPORT static WebPrescientNetworking* current(); None of these methods are needed. They should all be removed. Instead, we should get the WebPrescientNetworking instance from WebKit::Platform::current(). https://codereview.chromium.org/14746002/diff/43001/Source/Platform/chromium/... Source/Platform/chromium/public/WebPrescientNetworking.h:62: WebPrescientNetworking() { } No constructor is needed. https://codereview.chromium.org/14746002/diff/43001/Source/Platform/chromium/... Source/Platform/chromium/public/WebPrescientNetworking.h:66: static WebPrescientNetworking* s_platform; Please remove the static state from this class. Instead, this object should be retrieved from the WebKit::Platform::current static. https://codereview.chromium.org/14746002/diff/43001/Source/core/platform/chro... File Source/core/platform/chromium/support/WebPrescientNetworking.cpp (right): https://codereview.chromium.org/14746002/diff/43001/Source/core/platform/chro... Source/core/platform/chromium/support/WebPrescientNetworking.cpp:36: WebPrescientNetworking* WebPrescientNetworking::s_platform = 0; There's no need for this file. Please delete it. https://codereview.chromium.org/14746002/diff/43001/Source/core/platform/netw... File Source/core/platform/network/Preconnect.cpp (right): https://codereview.chromium.org/14746002/diff/43001/Source/core/platform/netw... Source/core/platform/network/Preconnect.cpp:42: WebKit::WebPrescientNetworking* platform = WebKit::WebPrescientNetworking::current(); WebKit::Platform::current()->precientNeworking() https://codereview.chromium.org/14746002/diff/43001/Source/core/platform/netw... File Source/core/platform/network/Preconnect.h (right): https://codereview.chromium.org/14746002/diff/43001/Source/core/platform/netw... Source/core/platform/network/Preconnect.h:44: }; There's no need for this enum. Code can just use the enum in the API directly. https://codereview.chromium.org/14746002/diff/43001/Source/core/platform/netw... Source/core/platform/network/Preconnect.h:46: void preconnect(const KURL&, PreconnectMotivation); There's no need for this file. Please delete it. Code can just call the Platform API directly.
Updated patch: access instance via Platform. nits fixed. https://chromiumcodereview.appspot.com/14746002/diff/43001/Source/Platform/ch... File Source/Platform/chromium/public/WebPrescientNetworking.h (right): https://chromiumcodereview.appspot.com/14746002/diff/43001/Source/Platform/ch... Source/Platform/chromium/public/WebPrescientNetworking.h:40: // FIXME (kouhei): Passing preconnect motivation to net is layering vioration. On 2013/05/14 06:18:56, abarth wrote: > " (kouhei)" <-- you should delete this part. We don't put user names in FIXME > comments in Blink. > > net -> "the platform". Blink has no knowledge that a "net" module exists. Done. https://chromiumcodereview.appspot.com/14746002/diff/43001/Source/Platform/ch... Source/Platform/chromium/public/WebPrescientNetworking.h:55: WEBKIT_EXPORT static WebPrescientNetworking* current(); On 2013/05/14 06:18:56, abarth wrote: > None of these methods are needed. They should all be removed. > > Instead, we should get the WebPrescientNetworking instance from > WebKit::Platform::current(). Done. https://chromiumcodereview.appspot.com/14746002/diff/43001/Source/Platform/ch... Source/Platform/chromium/public/WebPrescientNetworking.h:62: WebPrescientNetworking() { } On 2013/05/14 06:18:56, abarth wrote: > No constructor is needed. Done. https://chromiumcodereview.appspot.com/14746002/diff/43001/Source/Platform/ch... Source/Platform/chromium/public/WebPrescientNetworking.h:66: static WebPrescientNetworking* s_platform; On 2013/05/14 06:18:56, abarth wrote: > Please remove the static state from this class. Instead, this object should be > retrieved from the WebKit::Platform::current static. Done. https://chromiumcodereview.appspot.com/14746002/diff/43001/Source/core/platfo... File Source/core/platform/chromium/support/WebPrescientNetworking.cpp (right): https://chromiumcodereview.appspot.com/14746002/diff/43001/Source/core/platfo... Source/core/platform/chromium/support/WebPrescientNetworking.cpp:36: WebPrescientNetworking* WebPrescientNetworking::s_platform = 0; On 2013/05/14 06:18:56, abarth wrote: > There's no need for this file. Please delete it. Done. https://chromiumcodereview.appspot.com/14746002/diff/43001/Source/core/platfo... File Source/core/platform/network/Preconnect.cpp (right): https://chromiumcodereview.appspot.com/14746002/diff/43001/Source/core/platfo... Source/core/platform/network/Preconnect.cpp:42: WebKit::WebPrescientNetworking* platform = WebKit::WebPrescientNetworking::current(); removed this file. Done. https://chromiumcodereview.appspot.com/14746002/diff/43001/Source/core/platfo... File Source/core/platform/network/Preconnect.h (right): https://chromiumcodereview.appspot.com/14746002/diff/43001/Source/core/platfo... Source/core/platform/network/Preconnect.h:44: }; On 2013/05/14 06:18:56, abarth wrote: > There's no need for this enum. Code can just use the enum in the API directly. Done. https://chromiumcodereview.appspot.com/14746002/diff/43001/Source/core/platfo... Source/core/platform/network/Preconnect.h:46: void preconnect(const KURL&, PreconnectMotivation); On 2013/05/14 06:18:56, abarth wrote: > There's no need for this file. Please delete it. Code can just call the > Platform API directly. Done.
LGTM! Thanks for iterating on the CL. https://chromiumcodereview.appspot.com/14746002/diff/55002/Source/Platform/ch... File Source/Platform/chromium/public/Platform.h (right): https://chromiumcodereview.appspot.com/14746002/diff/55002/Source/Platform/ch... Source/Platform/chromium/public/Platform.h:272: virtual WebPrescientNetworking* prescientNetworking() { return 0; } Don't we need to forward declare WebPrescientNetworking for this to compile? https://chromiumcodereview.appspot.com/14746002/diff/55002/Source/Platform/ch... File Source/Platform/chromium/public/WebPrescientNetworking.h (right): https://chromiumcodereview.appspot.com/14746002/diff/55002/Source/Platform/ch... Source/Platform/chromium/public/WebPrescientNetworking.h:56: }; You might also need a virtual destructor to make some platforms happy.
Thank you for review! https://codereview.chromium.org/14746002/diff/55002/Source/Platform/chromium/... File Source/Platform/chromium/public/Platform.h (right): https://codereview.chromium.org/14746002/diff/55002/Source/Platform/chromium/... Source/Platform/chromium/public/Platform.h:272: virtual WebPrescientNetworking* prescientNetworking() { return 0; } On 2013/05/14 06:38:47, abarth wrote: > Don't we need to forward declare WebPrescientNetworking for this to compile? Done. https://codereview.chromium.org/14746002/diff/55002/Source/Platform/chromium/... File Source/Platform/chromium/public/WebPrescientNetworking.h (right): https://codereview.chromium.org/14746002/diff/55002/Source/Platform/chromium/... Source/Platform/chromium/public/WebPrescientNetworking.h:56: }; On 2013/05/14 06:38:47, abarth wrote: > You might also need a virtual destructor to make some platforms happy. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kouhei@chromium.org/14746002/58001
Message was sent while issue was closed.
Change committed as 150344 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
