Chromium Code Reviews
|
|
|
Created:
4 years, 11 months ago by Yoav Weiss Modified:
4 years, 11 months ago Reviewers:
Mike West CC:
blink-reviews, blink-reviews-html_chromium.org, dglazkov+blink, gavinp+loader_chromium.org, Nate Chapin, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd <link rel=preconnect> support to the HTMLPreloadScanner
This CL adds support for <link rel=preconnect> to the preloadScanner, to make sure that it is taken into account ASAP even if the parser is blocked.
BUG=450682
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196393
Patch Set 1 #
Total comments: 9
Patch Set 2 : review comments #Patch Set 3 : Removed preconnector. Passing preconnects through main thread. #
Total comments: 3
Patch Set 4 : limit the change to HTTP family #Patch Set 5 : Added HTTP family test #
Messages
Total messages: 24 (9 generated)
yoav@yoav.ws changed reviewers: + mkwst@chromium.org
Hey Mike :) Can you take a look?
LGTM % comments https://codereview.chromium.org/1152043005/diff/1/Source/core/core.gypi File Source/core/core.gypi (right): https://codereview.chromium.org/1152043005/diff/1/Source/core/core.gypi#newco... Source/core/core.gypi:1751: 'loader/Preconnecter.cpp', Nit: Preconnect_o_r. https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... File Source/core/html/parser/HTMLPreloadScanner.cpp (right): https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... Source/core/html/parser/HTMLPreloadScanner.cpp:175: // TODO(yoav): Add a real cross attribute value here once it's no longer a no-op. I see. Ok, yes, please add this TODO to the test as well so that it's clear that the current behavior isn't final. https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... Source/core/html/parser/HTMLPreloadScanner.cpp:311: isStyleSheet = rel.isStyleSheet() && !rel.isAlternate() && rel.iconType() == InvalidIcon && !rel.isDNSPrefetch(); Is this used anywhere else? I'd rather see it inlined into line 246 rather than doing the param-passing here. https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... Source/core/html/parser/HTMLPreloadScanner.cpp:349: return (match(m_tagImpl, linkTag) && m_linkIsPreconnect && !m_urlToLoad.isEmpty()); Nit: Drop the outer (). https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... File Source/core/html/parser/HTMLPreloadScannerTest.cpp (right): https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... Source/core/html/parser/HTMLPreloadScannerTest.cpp:37: class MockPreconnecter : public PreconnecterHost { Nit: I'd prefer either `MockPreconnecterHost : public PreconnecterHost` or `MockPreconnecter : public Preconnecter`. https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... Source/core/html/parser/HTMLPreloadScannerTest.cpp:64: Nit: No newline. https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... Source/core/html/parser/HTMLPreloadScannerTest.cpp:68: class MockHTMLResourcePreloader : public ResourcePreloader { Nit: Newline. https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... Source/core/html/parser/HTMLPreloadScannerTest.cpp:263: {"http://example.test", "<link rel=preconnected href=http://example2.test crossorigin='use-credentials'>", "", CrossOriginAttributeNotSet}, Is it intentional that the `crossorigin` attribute is always NotSet? If so, please add that to the CL description (and add a TODO?) https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... Source/core/html/parser/HTMLPreloadScannerTest.cpp:266: for (auto testCase : testCases) Nit: `const auto&`
On 2015/06/02 11:18:34, Mike West wrote: > LGTM % comments > > https://codereview.chromium.org/1152043005/diff/1/Source/core/core.gypi > File Source/core/core.gypi (right): > > https://codereview.chromium.org/1152043005/diff/1/Source/core/core.gypi#newco... > Source/core/core.gypi:1751: 'loader/Preconnecter.cpp', > Nit: Preconnect_o_r. Yeah. Should've switched to it after typing it as preconnector half the time. > > https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... > File Source/core/html/parser/HTMLPreloadScanner.cpp (right): > > https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... > Source/core/html/parser/HTMLPreloadScanner.cpp:175: // TODO(yoav): Add a real > cross attribute value here once it's no longer a no-op. > I see. Ok, yes, please add this TODO to the test as well so that it's clear that > the current behavior isn't final. Apologies for not making it clearer that crossorigin support is not yet there. > > https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... > Source/core/html/parser/HTMLPreloadScanner.cpp:311: isStyleSheet = > rel.isStyleSheet() && !rel.isAlternate() && rel.iconType() == InvalidIcon && > !rel.isDNSPrefetch(); > Is this used anywhere else? I'd rather see it inlined into line 246 rather than > doing the param-passing here. Changed it, but out of curiosity, why? > > https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... > Source/core/html/parser/HTMLPreloadScanner.cpp:349: return (match(m_tagImpl, > linkTag) && m_linkIsPreconnect && !m_urlToLoad.isEmpty()); > Nit: Drop the outer (). > > https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... > File Source/core/html/parser/HTMLPreloadScannerTest.cpp (right): > > https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... > Source/core/html/parser/HTMLPreloadScannerTest.cpp:37: class MockPreconnecter : > public PreconnecterHost { > Nit: I'd prefer either `MockPreconnecterHost : public PreconnecterHost` or > `MockPreconnecter : public Preconnecter`. > > https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... > Source/core/html/parser/HTMLPreloadScannerTest.cpp:64: > Nit: No newline. > > https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... > Source/core/html/parser/HTMLPreloadScannerTest.cpp:68: class > MockHTMLResourcePreloader : public ResourcePreloader { > Nit: Newline. > > https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... > Source/core/html/parser/HTMLPreloadScannerTest.cpp:263: {"http://example.test", > "<link rel=preconnected href=http://example2.test > crossorigin='use-credentials'>", "", CrossOriginAttributeNotSet}, > Is it intentional that the `crossorigin` attribute is always NotSet? If so, > please add that to the CL description (and add a TODO?) > > https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... > Source/core/html/parser/HTMLPreloadScannerTest.cpp:266: for (auto testCase : > testCases) > Nit: `const auto&`
On 2015/06/02 at 11:50:34, yoav wrote: > > https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... > > Source/core/html/parser/HTMLPreloadScanner.cpp:311: isStyleSheet = > > rel.isStyleSheet() && !rel.isAlternate() && rel.iconType() == InvalidIcon && > > !rel.isDNSPrefetch(); > > Is this used anywhere else? I'd rather see it inlined into line 246 rather than > > doing the param-passing here. > > Changed it, but out of curiosity, why? Because out-params drive me nuts. :) Especially since this is a one-off, it just hides the fact that you're assigning to those two variables. I found it confusing. *shrug*
On 2015/06/02 11:54:30, Mike West wrote: > On 2015/06/02 at 11:50:34, yoav wrote: > > > > https://codereview.chromium.org/1152043005/diff/1/Source/core/html/parser/HTM... > > > Source/core/html/parser/HTMLPreloadScanner.cpp:311: isStyleSheet = > > > rel.isStyleSheet() && !rel.isAlternate() && rel.iconType() == InvalidIcon && > > > !rel.isDNSPrefetch(); > > > Is this used anywhere else? I'd rather see it inlined into line 246 rather > than > > > doing the param-passing here. > > > > Changed it, but out of curiosity, why? > > Because out-params drive me nuts. :) Especially since this is a one-off, it just > hides the fact that you're assigning to those two variables. I found it > confusing. *shrug* Fair nuf :)
The CQ bit was checked by yoav@yoav.ws
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/1152043005/#ps20001 (title: "review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152043005/20001
The CQ bit was unchecked by yoav@yoav.ws
On 2015/06/02 12:02:14, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1152043005/20001 The approach I've taken here doesn't really work since I'm sending the preconnects too early, and there's no RenderThread on the Chrome end to actually initiate them. Sadly, we'd have to wait for the main thread for preconnects (just like for preloads). I'll change that and resubmit (possibly killing off the preconnector, since it won't be needed for testing if we go through the ResourcePreloader)
On 2015/06/03 04:49:04, Yoav Weiss wrote: > On 2015/06/02 12:02:14, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1152043005/20001 > > The approach I've taken here doesn't really work since I'm sending the > preconnects too early, and there's no RenderThread on the Chrome end to actually > initiate them. > Sadly, we'd have to wait for the main thread for preconnects (just like for > preloads). > I'll change that and resubmit (possibly killing off the preconnector, since it > won't be needed for testing if we go through the ResourcePreloader) Mike - can you take a second look?
On 2015/06/03 08:37:06, Yoav Weiss wrote: > On 2015/06/03 04:49:04, Yoav Weiss wrote: > > On 2015/06/02 12:02:14, commit-bot: I haz the power wrote: > > > CQ is trying da patch. Follow status at > > > https://chromium-cq-status.appspot.com/patch-status/1152043005/20001 > > > > The approach I've taken here doesn't really work since I'm sending the > > preconnects too early, and there's no RenderThread on the Chrome end to > actually > > initiate them. > > Sadly, we'd have to wait for the main thread for preconnects (just like for > > preloads). > > I'll change that and resubmit (possibly killing off the preconnector, since it > > won't be needed for testing if we go through the ResourcePreloader) > > Mike - can you take a second look? Also, I added support for crossOrigin while I was at it.
https://codereview.chromium.org/1152043005/diff/40001/Source/core/html/parser... File Source/core/html/parser/HTMLPreloadScannerTest.cpp (right): https://codereview.chromium.org/1152043005/diff/40001/Source/core/html/parser... Source/core/html/parser/HTMLPreloadScannerTest.cpp:50: if (!host.isNull()) { When would `host` be null? Can we simply ASSERT that it isn't? https://codereview.chromium.org/1152043005/diff/40001/Source/core/html/parser... File Source/core/html/parser/HTMLResourcePreloader.cpp (right): https://codereview.chromium.org/1152043005/diff/40001/Source/core/html/parser... Source/core/html/parser/HTMLResourcePreloader.cpp:57: if (!host.isValid()) Perhaps add `!host.protocolIsInHTTPFamily()`? We wouldn't want to preconnect to `data:`, for instance. What about `ws`/`wss`? Do we care about those? `ftp`? https://codereview.chromium.org/1152043005/diff/40001/Source/core/html/parser... Source/core/html/parser/HTMLResourcePreloader.cpp:62: crossOrigin = CrossOriginAttributeUseCredentials; Looking at this again, what does this actually mean in the context of preconnect? Do we do a preflight? Do we send cookies, etc? I thought this just warmed up the socket.
On 2015/06/03 09:02:27, Mike West wrote: > https://codereview.chromium.org/1152043005/diff/40001/Source/core/html/parser... > File Source/core/html/parser/HTMLPreloadScannerTest.cpp (right): > > https://codereview.chromium.org/1152043005/diff/40001/Source/core/html/parser... > Source/core/html/parser/HTMLPreloadScannerTest.cpp:50: if (!host.isNull()) { > When would `host` be null? Can we simply ASSERT that it isn't? host is null when the test case provides it as such (see line 241). It's just an indication that for this test, m_preloadRequest shouldn't be there at all. > > https://codereview.chromium.org/1152043005/diff/40001/Source/core/html/parser... > File Source/core/html/parser/HTMLResourcePreloader.cpp (right): > > https://codereview.chromium.org/1152043005/diff/40001/Source/core/html/parser... > Source/core/html/parser/HTMLResourcePreloader.cpp:57: if (!host.isValid()) > Perhaps add `!host.protocolIsInHTTPFamily()`? We wouldn't want to preconnect to > `data:`, for instance. What about `ws`/`wss`? Do we care about those? `ftp`? Good catch! HTTP family it is. > > https://codereview.chromium.org/1152043005/diff/40001/Source/core/html/parser... > Source/core/html/parser/HTMLResourcePreloader.cpp:62: crossOrigin = > CrossOriginAttributeUseCredentials; > Looking at this again, what does this actually mean in the context of > preconnect? Do we do a preflight? Do we send cookies, etc? I thought this just > warmed up the socket. Currently, it does nothing. It may continue to do nothing, in which case I'll rip that code out. The reason I added it is that currently anonymous resources use the "private mode" socket pool, so preconnect doesn't help those. The most prominent example is fonts. Since Firefox ran into the same issue, we though of solving it with a "crossorigin" However, Ryan disagreed with the "crossorigin" approach. For the full discussion, see https://codereview.chromium.org/1131293004/ Currently, Ryan and Ilya are talking to people and looking into possible alternatives, and if the winning one won't be a crossorigin attribute, I'll remove that support.
ok. LGTM. Again. :)
The CQ bit was checked by yoav@yoav.ws
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/1152043005/#ps60001 (title: "limit the change to HTTP family")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152043005/60001
The CQ bit was unchecked by yoav@yoav.ws
The CQ bit was checked by yoav@yoav.ws
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/1152043005/#ps80001 (title: "Added HTTP family test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152043005/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196393 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
