Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1577073005:
Add <link rel=preload> onload support for scripts and styles (Closed)
|
Created:
4 years, 11 months ago by Yoav Weiss Modified:
4 years, 9 months ago Reviewers:
Nate Chapin CC:
chromium-reviews, tyoshino+watch_chromium.org, gavinp+prerender_chromium.org, blink-reviews-style_chromium.org, blink-reviews-html_chromium.org, loading-reviews_chromium.org, dglazkov+blink, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, kinuko+watch, Nate Chapin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd <link rel=preload> onload support for scripts and styles
This CL adds onload support for <link rel=preload> for scripts and styles.
Support for other resource types will be added in a followup CL.
BUG=552289
Committed: https://crrev.com/f85424718ca7a8ea3f330141ee68b717ae02f2f0
Cr-Commit-Position: refs/heads/master@{#369892}
Patch Set 1 #Patch Set 2 : Fixed lifetime issue of LinkPreloadResourceClients #
Total comments: 17
Patch Set 3 : Review comments #
Total comments: 1
Patch Set 4 : Reversed order of base classes #Patch Set 5 : startPreload returns ResourcePtr #Patch Set 6 : fix crash #Patch Set 7 : Clearing resource in destructor #Patch Set 8 : Defeatedly removed image :/ #
Total comments: 15
Patch Set 9 : more review comments #Patch Set 10 : last review comments #Patch Set 11 : cleanup #Patch Set 12 : Fixed adoptPtr #Messages
Total messages: 23 (6 generated)
yoav@yoav.ws changed reviewers: + japhet@chromium.org
Hey Nate :) This is the result of our long irc discussions. Can you take a look?
On 2016/01/13 12:36:25, Yoav Weiss wrote: > Hey Nate :) > > This is the result of our long irc discussions. Can you take a look? test failures are relevant (and fixing them would require some significant changes). Don't review just yet
https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right): https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:123: Resource::checkNotify(); As long as CSSStyleSheetResource has its own special finish notification, we probably shouldn't send notifyFinished(), which calling Resource::checkNotify() will do. https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:164: void DocumentLoader::startPreload(Resource::Type type, FetchRequest& request, LinkLoader* linkLoader) It seems like it would be cleaner to change startPreload() to return a ResourcePtr<> here and let LinkLoader set up the client. https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkLoader.cpp:192: static void preloadIfNeeded(const LinkRelAttribute& relAttribute, const KURL& href, Document& document, const String& as, LinkLoader* linkLoader) Should this be a private member of LinkLoader if we're going to start accessing LinkLoader properties? https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkLoader.cpp:236: preloadIfNeeded(relAttribute, url, *document, header.as(), nullptr); Passing nullptr in this case is not immediately obvious (and doesn't work if preloadIfNeeded becomes a LinkLoader member function). Maybe pass an enum value indicating whether this preload should have an onload event? https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h (right): https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:23: virtual void callSetResource(Resource*) = 0; Maybe take the Resource* as a parameter in the various create()/constructor functions instead of needing this virutal? https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:28: WeakPtr<LinkLoader> m_loader; LinkLoader owns LinkPreloadResourceClient. Wouldn't m_loader only get cleared when LinkPreloadResourceClient gets deleted? https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:31: class LinkPreloadScriptResourceClient: public ResourceOwner<Resource, ScriptResourceClient>, public LinkPreloadResourceClient { Here and below: If I understand oilpan correctly, the oilpan-heap superclass should be listed first, so LinkPreloadResourceClient should come before ResourceOwner. https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:45: clearResource(); Why does notifyFinished() call clearResource() in LinkPreloadScriptResourceClient, but not the other LinkPreloadResourceClients?
Thanks for reviewing! :) Addressed some of the comments and replied to the ones that I didn't change. Let me know what you thing https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right): https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:123: Resource::checkNotify(); On 2016/01/13 23:52:23, Nate Chapin wrote: > As long as CSSStyleSheetResource has its own special finish notification, we > probably shouldn't send notifyFinished(), which calling Resource::checkNotify() > will do. Previous iterations made me believe that this is necessary to get the stylesheet onload event working. Testing it now without it shows that its not the case. https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:164: void DocumentLoader::startPreload(Resource::Type type, FetchRequest& request, LinkLoader* linkLoader) On 2016/01/13 23:52:23, Nate Chapin wrote: > It seems like it would be cleaner to change startPreload() to return a > ResourcePtr<> here and let LinkLoader set up the client. Returning things to LinkLoader makes the call from LinkLoader significantly more complex and forces other call sites of startPreload (HTMLResourcePreloader) to know the size of LinkPreloadResourceClient. So it's easier to keep setting the resourceClient here. OTOH, I did clean up things quite a bit. Let me know if it works, and if not, I'll change this to return the ResourceClient over. https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkLoader.cpp:192: static void preloadIfNeeded(const LinkRelAttribute& relAttribute, const KURL& href, Document& document, const String& as, LinkLoader* linkLoader) On 2016/01/13 23:52:23, Nate Chapin wrote: > Should this be a private member of LinkLoader if we're going to start accessing > LinkLoader properties? This has to remain static since it's called from loadLinkFromHeader, which is called without a LinkLoader. So, we would still have to pass linkLoader around, even if this was a member. https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkLoader.cpp:236: preloadIfNeeded(relAttribute, url, *document, header.as(), nullptr); On 2016/01/13 23:52:23, Nate Chapin wrote: > Passing nullptr in this case is not immediately obvious (and doesn't work if > preloadIfNeeded becomes a LinkLoader member function). Maybe pass an enum value > indicating whether this preload should have an onload event? If preloadIfNeeded would not be static, we wouldn't be able to call it from here. https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h (right): https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:23: virtual void callSetResource(Resource*) = 0; On 2016/01/13 23:52:24, Nate Chapin wrote: > Maybe take the Resource* as a parameter in the various create()/constructor > functions instead of needing this virutal? good call! :) https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:28: WeakPtr<LinkLoader> m_loader; On 2016/01/13 23:52:23, Nate Chapin wrote: > LinkLoader owns LinkPreloadResourceClient. Wouldn't m_loader only get cleared > when LinkPreloadResourceClient gets deleted? true. Previousely left the weakPtr around since the alternative is a raw pointer (otherwise we have a circular ref), but maybe a raw pointer is better. Changed. https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:31: class LinkPreloadScriptResourceClient: public ResourceOwner<Resource, ScriptResourceClient>, public LinkPreloadResourceClient { On 2016/01/13 23:52:23, Nate Chapin wrote: > Here and below: If I understand oilpan correctly, the oilpan-heap superclass > should be listed first, so LinkPreloadResourceClient should come before > ResourceOwner. ok https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:45: clearResource(); On 2016/01/13 23:52:24, Nate Chapin wrote: > Why does notifyFinished() call clearResource() in > LinkPreloadScriptResourceClient, but not the other LinkPreloadResourceClients? leftover. Apologies.
On 2016/01/14 10:10:02, Yoav Weiss wrote: > Thanks for reviewing! :) Addressed some of the comments and replied to the ones > that I didn't change. Let me know what you thing > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right): > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:123: > Resource::checkNotify(); > On 2016/01/13 23:52:23, Nate Chapin wrote: > > As long as CSSStyleSheetResource has its own special finish notification, we > > probably shouldn't send notifyFinished(), which calling > Resource::checkNotify() > > will do. > > Previous iterations made me believe that this is necessary to get the stylesheet > onload event working. Testing it now without it shows that its not the case. > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/DocumentLoader.cpp:164: void > DocumentLoader::startPreload(Resource::Type type, FetchRequest& request, > LinkLoader* linkLoader) > On 2016/01/13 23:52:23, Nate Chapin wrote: > > It seems like it would be cleaner to change startPreload() to return a > > ResourcePtr<> here and let LinkLoader set up the client. > > Returning things to LinkLoader makes the call from LinkLoader significantly more > complex and forces other call sites of startPreload (HTMLResourcePreloader) to > know the size of LinkPreloadResourceClient. So it's easier to keep setting the > resourceClient here. > > OTOH, I did clean up things quite a bit. Let me know if it works, and if not, > I'll change this to return the ResourceClient over. > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/LinkLoader.cpp:192: static void > preloadIfNeeded(const LinkRelAttribute& relAttribute, const KURL& href, > Document& document, const String& as, LinkLoader* linkLoader) > On 2016/01/13 23:52:23, Nate Chapin wrote: > > Should this be a private member of LinkLoader if we're going to start > accessing > > LinkLoader properties? > > This has to remain static since it's called from loadLinkFromHeader, which is > called without a LinkLoader. > So, we would still have to pass linkLoader around, even if this was a member. > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/LinkLoader.cpp:236: > preloadIfNeeded(relAttribute, url, *document, header.as(), nullptr); > On 2016/01/13 23:52:23, Nate Chapin wrote: > > Passing nullptr in this case is not immediately obvious (and doesn't work if > > preloadIfNeeded becomes a LinkLoader member function). Maybe pass an enum > value > > indicating whether this preload should have an onload event? > > If preloadIfNeeded would not be static, we wouldn't be able to call it from > here. > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h (right): > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:23: virtual > void callSetResource(Resource*) = 0; > On 2016/01/13 23:52:24, Nate Chapin wrote: > > Maybe take the Resource* as a parameter in the various create()/constructor > > functions instead of needing this virutal? > > good call! :) > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:28: > WeakPtr<LinkLoader> m_loader; > On 2016/01/13 23:52:23, Nate Chapin wrote: > > LinkLoader owns LinkPreloadResourceClient. Wouldn't m_loader only get cleared > > when LinkPreloadResourceClient gets deleted? > > true. Previousely left the weakPtr around since the alternative is a raw pointer > (otherwise we have a circular ref), but maybe a raw pointer is better. Changed. > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:31: class > LinkPreloadScriptResourceClient: public ResourceOwner<Resource, > ScriptResourceClient>, public LinkPreloadResourceClient { > On 2016/01/13 23:52:23, Nate Chapin wrote: > > Here and below: If I understand oilpan correctly, the oilpan-heap superclass > > should be listed first, so LinkPreloadResourceClient should come before > > ResourceOwner. > > ok > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:45: > clearResource(); > On 2016/01/13 23:52:24, Nate Chapin wrote: > > Why does notifyFinished() call clearResource() in > > LinkPreloadScriptResourceClient, but not the other LinkPreloadResourceClients? > > leftover. Apologies. s/thing/think! (DYAC)
hmm, ASAN says that the imageResources (and only the image related ones) are leaking...
I don't see an obvious explanation for the memory leak. I would wonder if ResourceFetcher's preload set or MemoryCache needs to be cleared, but I don't see any reason why this patch would cause a leak to only start now if that were the problem... https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:164: void DocumentLoader::startPreload(Resource::Type type, FetchRequest& request, LinkLoader* linkLoader) On 2016/01/14 10:10:02, Yoav Weiss wrote: > On 2016/01/13 23:52:23, Nate Chapin wrote: > > It seems like it would be cleaner to change startPreload() to return a > > ResourcePtr<> here and let LinkLoader set up the client. > > Returning things to LinkLoader makes the call from LinkLoader significantly more > complex and forces other call sites of startPreload (HTMLResourcePreloader) to > know the size of LinkPreloadResourceClient. So it's easier to keep setting the > resourceClient here. > > OTOH, I did clean up things quite a bit. Let me know if it works, and if not, > I'll change this to return the ResourceClient over. I don't see how HTMLResourcePreload would need to know anything about LinkPreloadResourceClient? LinkLoader knows the Resource::Type, so it knows which (if any) LinkPreloadResourceClient is needed. Suppose DocumentLoader::startPreload() and LinkLoader.cpp's preloadIfNeeded() are changed to return a ResourcePtr<Resource>. LinkLoader::loadLink() can check whether ResourcePtr<Resource> was in fact non-null and create the appropriate LinkPreloadResourceClient with the Resource. Does that not work for some reason? https://codereview.chromium.org/1577073005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h (right): https://codereview.chromium.org/1577073005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:87: class LinkPreloadImageResourceClient: public ResourceOwner<Resource, ImageResourceClient>, public LinkPreloadResourceClient { Looks like you missed this one for reversing the superclass ordering.
On 2016/01/14 23:10:53, Nate Chapin wrote: > I don't see an obvious explanation for the memory leak. I would wonder if > ResourceFetcher's preload set or MemoryCache needs to be cleared, but I don't > see any reason why this patch would cause a leak to only start now if that were > the problem... > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/DocumentLoader.cpp:164: void > DocumentLoader::startPreload(Resource::Type type, FetchRequest& request, > LinkLoader* linkLoader) > On 2016/01/14 10:10:02, Yoav Weiss wrote: > > On 2016/01/13 23:52:23, Nate Chapin wrote: > > > It seems like it would be cleaner to change startPreload() to return a > > > ResourcePtr<> here and let LinkLoader set up the client. > > > > Returning things to LinkLoader makes the call from LinkLoader significantly > more > > complex and forces other call sites of startPreload (HTMLResourcePreloader) to > > know the size of LinkPreloadResourceClient. So it's easier to keep setting the > > resourceClient here. > > > > OTOH, I did clean up things quite a bit. Let me know if it works, and if not, > > I'll change this to return the ResourceClient over. > > I don't see how HTMLResourcePreload would need to know anything about > LinkPreloadResourceClient? LinkLoader knows the Resource::Type, so it knows > which (if any) LinkPreloadResourceClient is needed. Suppose > DocumentLoader::startPreload() and LinkLoader.cpp's preloadIfNeeded() are > changed to return a ResourcePtr<Resource>. LinkLoader::loadLink() can check > whether ResourcePtr<Resource> was in fact non-null and create the appropriate > LinkPreloadResourceClient with the Resource. Does that not work for some reason? Fair enough. I was trying to avoid duplicating the switch statement on LinkLoader's side, but now changed it so that DocumentLoader is not aware of LinkPreloadResourceClient. > > https://codereview.chromium.org/1577073005/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h (right): > > https://codereview.chromium.org/1577073005/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:87: class > LinkPreloadImageResourceClient: public ResourceOwner<Resource, > ImageResourceClient>, public LinkPreloadResourceClient { > Looks like you missed this one for reversing the superclass ordering. Good catch! :) Maybe this is the source of the mysterious leak?
On 2016/01/15 09:20:19, Yoav Weiss wrote: > On 2016/01/14 23:10:53, Nate Chapin wrote: > > I don't see an obvious explanation for the memory leak. I would wonder if > > ResourceFetcher's preload set or MemoryCache needs to be cleared, but I don't > > see any reason why this patch would cause a leak to only start now if that > were > > the problem... > > > > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): > > > > > https://codereview.chromium.org/1577073005/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/loader/DocumentLoader.cpp:164: void > > DocumentLoader::startPreload(Resource::Type type, FetchRequest& request, > > LinkLoader* linkLoader) > > On 2016/01/14 10:10:02, Yoav Weiss wrote: > > > On 2016/01/13 23:52:23, Nate Chapin wrote: > > > > It seems like it would be cleaner to change startPreload() to return a > > > > ResourcePtr<> here and let LinkLoader set up the client. > > > > > > Returning things to LinkLoader makes the call from LinkLoader significantly > > more > > > complex and forces other call sites of startPreload (HTMLResourcePreloader) > to > > > know the size of LinkPreloadResourceClient. So it's easier to keep setting > the > > > resourceClient here. > > > > > > OTOH, I did clean up things quite a bit. Let me know if it works, and if > not, > > > I'll change this to return the ResourceClient over. > > > > I don't see how HTMLResourcePreload would need to know anything about > > LinkPreloadResourceClient? LinkLoader knows the Resource::Type, so it knows > > which (if any) LinkPreloadResourceClient is needed. Suppose > > DocumentLoader::startPreload() and LinkLoader.cpp's preloadIfNeeded() are > > changed to return a ResourcePtr<Resource>. LinkLoader::loadLink() can check > > whether ResourcePtr<Resource> was in fact non-null and create the appropriate > > LinkPreloadResourceClient with the Resource. Does that not work for some > reason? > > Fair enough. I was trying to avoid duplicating the switch statement on > LinkLoader's side, but now changed it so that DocumentLoader is not aware of > LinkPreloadResourceClient. > > > > > > https://codereview.chromium.org/1577073005/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h > (right): > > > > > https://codereview.chromium.org/1577073005/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:87: class > > LinkPreloadImageResourceClient: public ResourceOwner<Resource, > > ImageResourceClient>, public LinkPreloadResourceClient { > > Looks like you missed this one for reversing the superclass ordering. > > Good catch! :) Maybe this is the source of the mysterious leak? After failing to find the cause for the leak for now, I prefer to land these changes without supporting images, and add leak-free image support in a followup CL. WDYT?
Looking pretty good, just a few more nits. https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:163: ResourcePtr<Resource> DocumentLoader::startPreload(Resource::Type type, FetchRequest& request, LinkLoader* linkLoader) linkLoader parameter looks unused. https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.h:59: class LinkLoader; This can go away if startPreload() doesn't need a LinkLoader parameter. https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkLoader.cpp:191: static void createLinkPreloadResourceClient(ResourcePtr<Resource> resource, LinkLoader* linkLoader, Resource::Type type) This could either return the LinkPreloadResourceClient or be a LinkLoader member, so that setPreloadResourceClient doesn't need to be public. https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkLoader.cpp:243: createLinkPreloadResourceClient(document.loader()->startPreload(type, linkRequest, linkLoader), linkLoader, type); Maybe do this after preloadIfNeeded is called in loadLink()? https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h (right): https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:35: class LinkPreloadScriptResourceClient: public LinkPreloadResourceClient, public ResourceOwner<Resource, ScriptResourceClient> { Maybe just hold this as a ResourceOwner<ScriptResource> for clarity? https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:60: class LinkPreloadStyleResourceClient: public LinkPreloadResourceClient, public ResourceOwner<Resource, StyleSheetResourceClient> { Ditto here and CSSStyleSheetResource. https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:71: void notifyFinished(Resource* resource) override Should this be setCSSStyleSheet()?
https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:163: ResourcePtr<Resource> DocumentLoader::startPreload(Resource::Type type, FetchRequest& request, LinkLoader* linkLoader) On 2016/01/15 18:53:58, Nate Chapin wrote: > linkLoader parameter looks unused. yup https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.h (right): https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.h:59: class LinkLoader; On 2016/01/15 18:53:58, Nate Chapin wrote: > This can go away if startPreload() doesn't need a LinkLoader parameter. true https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkLoader.cpp:191: static void createLinkPreloadResourceClient(ResourcePtr<Resource> resource, LinkLoader* linkLoader, Resource::Type type) On 2016/01/15 18:53:58, Nate Chapin wrote: > This could either return the LinkPreloadResourceClient or be a LinkLoader > member, so that setPreloadResourceClient doesn't need to be public. yeah, adding both this and preloadIfNeeded as statics on LinkLoader, to avoid publics https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkLoader.cpp:243: createLinkPreloadResourceClient(document.loader()->startPreload(type, linkRequest, linkLoader), linkLoader, type); On 2016/01/15 18:53:58, Nate Chapin wrote: > Maybe do this after preloadIfNeeded is called in loadLink()? not sure what you mean by that. Can you elaborate? https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h (right): https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:35: class LinkPreloadScriptResourceClient: public LinkPreloadResourceClient, public ResourceOwner<Resource, ScriptResourceClient> { On 2016/01/15 18:53:58, Nate Chapin wrote: > Maybe just hold this as a ResourceOwner<ScriptResource> for clarity? sure https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:60: class LinkPreloadStyleResourceClient: public LinkPreloadResourceClient, public ResourceOwner<Resource, StyleSheetResourceClient> { On 2016/01/15 18:53:58, Nate Chapin wrote: > Ditto here and CSSStyleSheetResource. ok https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:71: void notifyFinished(Resource* resource) override On 2016/01/15 18:53:58, Nate Chapin wrote: > Should this be setCSSStyleSheet()? changed
LGTM https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkLoader.cpp:243: createLinkPreloadResourceClient(document.loader()->startPreload(type, linkRequest, linkLoader), linkLoader, type); On 2016/01/15 21:40:25, Yoav Weiss wrote: > On 2016/01/15 18:53:58, Nate Chapin wrote: > > Maybe do this after preloadIfNeeded is called in loadLink()? > > not sure what you mean by that. Can you elaborate? createLinkPreloadResourceClient() only does work if linkLoader is non-null, which is only true when preloadIfNeeded() is called from loadLink(). Calling createLinkPreloadResourceClient() from loadLink() means we don't need to null check. It also means preloadIfNeeded() doesn't need a LinkLoader* parameter.
Description was changed from ========== Add onload support for <link rel=preload> This CL adds onload support for <link rel=preload> for certain resource types: script, style and image. Support for other resource types will be added in a followup CL. BUG=552289 ========== to ========== Add <link rel=preload> onload support for scripts and styles This CL adds onload support for <link rel=preload> for scripts and styles. Support for other resource types will be added in a followup CL. BUG=552289 ==========
On 2016/01/15 21:52:44, Nate Chapin wrote: > LGTM > > https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): > > https://codereview.chromium.org/1577073005/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/LinkLoader.cpp:243: > createLinkPreloadResourceClient(document.loader()->startPreload(type, > linkRequest, linkLoader), linkLoader, type); > On 2016/01/15 21:40:25, Yoav Weiss wrote: > > On 2016/01/15 18:53:58, Nate Chapin wrote: > > > Maybe do this after preloadIfNeeded is called in loadLink()? > > > > not sure what you mean by that. Can you elaborate? > > createLinkPreloadResourceClient() only does work if linkLoader is non-null, > which is only true when preloadIfNeeded() is called from loadLink(). Calling > createLinkPreloadResourceClient() from loadLink() means we don't need to null > check. It also means preloadIfNeeded() doesn't need a LinkLoader* parameter. Good call. Moved it out and the result is much cleaner. Thanks! :)
The CQ bit was checked by yoav@yoav.ws
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/1577073005/#ps200001 (title: "cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1577073005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1577073005/200001
Message was sent while issue was closed.
Description was changed from ========== Add <link rel=preload> onload support for scripts and styles This CL adds onload support for <link rel=preload> for scripts and styles. Support for other resource types will be added in a followup CL. BUG=552289 ========== to ========== Add <link rel=preload> onload support for scripts and styles This CL adds onload support for <link rel=preload> for scripts and styles. Support for other resource types will be added in a followup CL. BUG=552289 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add <link rel=preload> onload support for scripts and styles This CL adds onload support for <link rel=preload> for scripts and styles. Support for other resource types will be added in a followup CL. BUG=552289 ========== to ========== Add <link rel=preload> onload support for scripts and styles This CL adds onload support for <link rel=preload> for scripts and styles. Support for other resource types will be added in a followup CL. BUG=552289 Committed: https://crrev.com/f85424718ca7a8ea3f330141ee68b717ae02f2f0 Cr-Commit-Position: refs/heads/master@{#369892} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/f85424718ca7a8ea3f330141ee68b717ae02f2f0 Cr-Commit-Position: refs/heads/master@{#369892}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/1595793002/ by samuong@chromium.org. The reason for reverting is: it broke the compile step on the WebKit Linux Oilpan Builder: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%.... |
