Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2072613002:
Make ResourceLoadPriority calculation simpler (Closed)
|
Created:
4 years, 6 months ago by Nate Chapin Modified:
4 years, 4 months ago 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake ResourceLoadPriority calculation simpler
* Don't keep a separate ResourceLoadPriority value on
FetchRequest, just use ResourceRequest::m_priority.
* Make a bit more use of FetchRequest::LazyLoad for cases
like preloads where the priority of a request should be
very low.
* ResourceFetcher::loadPriority() shouldn't blindly trust the
priority given by a caller, and instead takes the max of
the provided priority and the priority it calculates.
* Add a helper to FetchRequest to make a request synchronous,
so callers don't need to manually flag the request as
Highest priority.
This may or may not fix a case where synchronous requests are
incorrectly and flakily being given a priority lower than
Highest.
BUG=620334
Committed: https://crrev.com/fd0671f01914a4f5e74620e75f673f213d302823
Cr-Commit-Position: refs/heads/master@{#410216}
Patch Set 1 #Patch Set 2 : Drop a default arg #
Total comments: 8
Patch Set 3 : Address kinuko's comments #Patch Set 4 : +comment re: max() #Patch Set 5 : Make the comment look more like English #
Total comments: 13
Patch Set 6 : Address review comments #
Total comments: 2
Patch Set 7 : Rebase #Messages
Total messages: 39 (17 generated)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2072613002/20001
Description was changed from ========== Make ResourceLoadPriority calculation simpler This may or may not fix a case where synchronous requests are incorrectly being given a priority lower than Highest. BUG=620334 ========== to ========== Make ResourceLoadPriority calculation simpler * Don't keep a separate ResourceLoadPriority value on FetchRequest, just use ResourceRequest::m_priority. * Make a bit more use of FetchRequest::LazyLoad for cases like preloads where the priority of a request should be very low. * ResourceFetcher::loadPriority() shouldn't blindly trust the priority given by a caller, and instead takes the max of the provided priority and the priority it calculates. This may or may not fix a case where synchronous requests are incorrectly and flakily being given a priority lower than Highest. BUG=620334 ==========
Description was changed from ========== Make ResourceLoadPriority calculation simpler * Don't keep a separate ResourceLoadPriority value on FetchRequest, just use ResourceRequest::m_priority. * Make a bit more use of FetchRequest::LazyLoad for cases like preloads where the priority of a request should be very low. * ResourceFetcher::loadPriority() shouldn't blindly trust the priority given by a caller, and instead takes the max of the provided priority and the priority it calculates. This may or may not fix a case where synchronous requests are incorrectly and flakily being given a priority lower than Highest. BUG=620334 ========== to ========== Make ResourceLoadPriority calculation simpler * Don't keep a separate ResourceLoadPriority value on FetchRequest, just use ResourceRequest::m_priority. * Make a bit more use of FetchRequest::LazyLoad for cases like preloads where the priority of a request should be very low. * ResourceFetcher::loadPriority() shouldn't blindly trust the priority given by a caller, and instead takes the max of the provided priority and the priority it calculates. * Add a helper to FetchRequest to make a request synchronous, so callers don't need to manually flag the request as Highest priority. This may or may not fix a case where synchronous requests are incorrectly and flakily being given a priority lower than Highest. BUG=620334 ==========
japhet@chromium.org changed reviewers: + yoav@yoav.ws
yoav: I'm removing a couple of your TODOs, so I figured you'd be a good reviewer for this :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Some drive-by comments/questions (looking good!) https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/FetchRequest.h (right): https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/FetchRequest.h:60: explicit FetchRequest(const ResourceRequest&, const AtomicString& initiator, const String& charset = String()); nit: this probably no need to be explicit https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:164: return std::max(context().modifyPriorityForExperiments(priority), request.resourceRequest().priority()); Is the max here to cover synchronous load case? https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:157: ResourceLoadPriority loadPriority(Resource::Type, const FetchRequest&, ResourcePriority::VisibilityStatus); nit: while you're here can we rename this to computeLoadPriority or something that's less confusing? (Is 'load' verb here?)
https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/FetchRequest.h (right): https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/FetchRequest.h:60: explicit FetchRequest(const ResourceRequest&, const AtomicString& initiator, const String& charset = String()); On 2016/06/17 05:40:47, kinuko wrote: > nit: this probably no need to be explicit Done. https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:164: return std::max(context().modifyPriorityForExperiments(priority), request.resourceRequest().priority()); On 2016/06/17 05:40:47, kinuko wrote: > Is the max here to cover synchronous load case? That's half of it. Also, it covers the case where an image moves into the viewport, then out of it, which would drop the priority this function reports. https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:157: ResourceLoadPriority loadPriority(Resource::Type, const FetchRequest&, ResourcePriority::VisibilityStatus); On 2016/06/17 05:40:47, kinuko wrote: > nit: while you're here can we rename this to computeLoadPriority or something > that's less confusing? (Is 'load' verb here?) load is a noun here, I think. The name reads like a simple accessor, but it's neither simple nor an accessor :/ computeLoadPriority() sounds good.
Thanks, this l-g-t-m =) https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:164: return std::max(context().modifyPriorityForExperiments(priority), request.resourceRequest().priority()); On 2016/06/17 18:38:16, Nate Chapin wrote: > On 2016/06/17 05:40:47, kinuko wrote: > > Is the max here to cover synchronous load case? > > That's half of it. Also, it covers the case where an image moves into the > viewport, then out of it, which would drop the priority this function reports. Ah I see, thanks. Would it make sense to add a brief comment about that? That sounds helpful.
https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2072613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:164: return std::max(context().modifyPriorityForExperiments(priority), request.resourceRequest().priority()); On 2016/06/20 04:28:31, kinuko wrote: > On 2016/06/17 18:38:16, Nate Chapin wrote: > > On 2016/06/17 05:40:47, kinuko wrote: > > > Is the max here to cover synchronous load case? > > > > That's half of it. Also, it covers the case where an image moves into the > > viewport, then out of it, which would drop the priority this function reports. > > Ah I see, thanks. Would it make sense to add a brief comment about that? That > sounds helpful. Done.
yoav: friendly ping :)
yoav@yoav.ws changed reviewers: + pmeenan@chromium.org
Apologies for the delayed response. On the roads... Refactoring looks good. Several questions regarding the functional changes below. Adding Pat as he's been doing some priority work and may have opinions :) https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:162: priority = ResourceLoadPriorityVeryLow; If I understand it correctly, this would mean that deferred and async scripts will now be downloaded with very low priority. Is that the case? If so, is this intentional? Seems like a significant change from current behavior. https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:168: return std::max(context().modifyPriorityForExperiments(priority), request.resourceRequest().priority()); In the image case, the "priority churn" can also have advantages, if the user flicks through some images which we haven't yet downloaded, it might be better to give higher priority to the ones that are in current viewport over ones in past viewports. Is there a cost to said churn? https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/LinkResource.cpp (right): https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/LinkResource.cpp:82: request.setDefer(FetchRequest::LazyLoad); AFAICT, this mainly impacts non-matching styles & alternate links, marking them as lazy, which will effectively result in them being of very low priority (and therefore no functional change here). Is that a correct analysis? https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkLoader.cpp:301: linkRequest.setDefer(FetchRequest::LazyLoad); Why should we mark LinkPreload resources as LazyLoad? They should be a functional equivalent of XHRs. https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp (left): https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp:109: // TODO(yoav): subresource should be *very* low priority (rather than low). I now understand why you made LinkPreload lazy loaded :) FWIW, I'm not sure I was right with that TODO.
https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:162: priority = ResourceLoadPriorityVeryLow; On 2016/06/25 08:37:59, Yoav Weiss wrote: > If I understand it correctly, this would mean that deferred and async scripts > will now be downloaded with very low priority. Is that the case? If so, is this > intentional? Seems like a significant change from current behavior. Scripts go through the first clause and shouldn't be changed. This basically just makes anything else that gets tagged as defer load at veryLow (AFAIK, scripts previously were the only thing with the LazyLoad setting so this shouldn't change any existing behavior. https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:168: return std::max(context().modifyPriorityForExperiments(priority), request.resourceRequest().priority()); On 2016/06/25 08:37:59, Yoav Weiss wrote: > In the image case, the "priority churn" can also have advantages, if the user > flicks through some images which we haven't yet downloaded, it might be better > to give higher priority to the ones that are in current viewport over ones in > past viewports. Is there a cost to said churn? We had logic previously to prevent exactly this churn because it also applies to the same image being reference in the viewport as well as late in the viewport. We want to make sure not to incorrectly decrease the priority of a resource that is still in the viewport. Dealing with the fast scroll case would actually be new behavior (and should be done carefully if we want to bother). https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkLoader.cpp:301: linkRequest.setDefer(FetchRequest::LazyLoad); On 2016/06/25 08:37:59, Yoav Weiss wrote: > Why should we mark LinkPreload resources as LazyLoad? They should be a > functional equivalent of XHRs. +1. I think you may have been thinking about prefetch (which is a hint and low priority) vs preload (which is a declarative fetch). It may also be confusing because we use "forPreload" internally as being an indicator that it was from the preload scanner (and not a link rel=preload).
The CQ bit was checked by japhet@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/2072613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:162: priority = ResourceLoadPriorityVeryLow; On 2016/06/27 14:56:55, Pat Meenan wrote: > On 2016/06/25 08:37:59, Yoav Weiss wrote: > > If I understand it correctly, this would mean that deferred and async scripts > > will now be downloaded with very low priority. Is that the case? If so, is > this > > intentional? Seems like a significant change from current behavior. > > Scripts go through the first clause and shouldn't be changed. This basically > just makes anything else that gets tagged as defer load at veryLow (AFAIK, > scripts previously were the only thing with the LazyLoad setting so this > shouldn't change any existing behavior. Correct. https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:168: return std::max(context().modifyPriorityForExperiments(priority), request.resourceRequest().priority()); On 2016/06/27 14:56:55, Pat Meenan wrote: > On 2016/06/25 08:37:59, Yoav Weiss wrote: > > In the image case, the "priority churn" can also have advantages, if the user > > flicks through some images which we haven't yet downloaded, it might be better > > to give higher priority to the ones that are in current viewport over ones in > > past viewports. Is there a cost to said churn? > > We had logic previously to prevent exactly this churn because it also applies to > the same image being reference in the viewport as well as late in the viewport. > We want to make sure not to incorrectly decrease the priority of a resource that > is still in the viewport. Dealing with the fast scroll case would actually be > new behavior (and should be done carefully if we want to bother). I added a bit more detail to this comment, please review for clarity/correctness :) https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/LinkResource.cpp (right): https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/LinkResource.cpp:82: request.setDefer(FetchRequest::LazyLoad); On 2016/06/25 08:37:59, Yoav Weiss wrote: > AFAICT, this mainly impacts non-matching styles & alternate links, marking them > as lazy, which will effectively result in them being of very low priority (and > therefore no functional change here). Is that a correct analysis? Yes, that's my understanding. https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkLoader.cpp:301: linkRequest.setDefer(FetchRequest::LazyLoad); On 2016/06/27 14:56:55, Pat Meenan wrote: > On 2016/06/25 08:37:59, Yoav Weiss wrote: > > Why should we mark LinkPreload resources as LazyLoad? They should be a > > functional equivalent of XHRs. > > +1. I think you may have been thinking about prefetch (which is a hint and low > priority) vs preload (which is a declarative fetch). It may also be confusing > because we use "forPreload" internally as being an indicator that it was from > the preload scanner (and not a link rel=preload). Ok, so the comment in LinkLoaderTest says rel=preload should be priority VeryLow, the test actually asserts that it should be Low, and this comment thread it should be functionally equivalent to XHRs, which are priority High. Which would you like? :) Updated the LazyLoad logic in computeLoadPriority() to make Raw LazyLoads Low instead of VeryLow, to keep the current behavior. Feel free to override that default action :) https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp (left): https://codereview.chromium.org/2072613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp:109: // TODO(yoav): subresource should be *very* low priority (rather than low). On 2016/06/25 08:37:59, Yoav Weiss wrote: > I now understand why you made LinkPreload lazy loaded :) FWIW, I'm not sure I > was right with that TODO. Reverted the changes in this file. I'll let you decide what to do with this TODO.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2072613002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): https://codereview.chromium.org/2072613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkLoader.cpp:301: linkRequest.setDefer(FetchRequest::LazyLoad); I think you left that one out, as we said LinkPreload shouldn't be LazyLoad. Also - how are tests passing with this change? :| In general, I'll try to see if we can remove LinkPreload as a type entirely (and just set it to whatever XHR is). Also, I'm assuming that you deleted the `setPriority()` as it's no longer needed, since priority is now set on the resource. Is that correct?
Sorry for the delay, catching up post-vacation... https://codereview.chromium.org/2072613002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): https://codereview.chromium.org/2072613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkLoader.cpp:301: linkRequest.setDefer(FetchRequest::LazyLoad); On 2016/06/30 09:09:14, Yoav Weiss wrote: > I think you left that one out, as we said LinkPreload shouldn't be LazyLoad. > Also - how are tests passing with this change? :| The tests currently expect LinkPreloads to be priority Low, instead of the usual priority High for XHRs and other Raw resources. ResourceFetcher now special-cases Raw LazyLoads to get that behavior. If you'd like me to change what the tests expect instead, I'm happy to do that, but I was aiming to keep this cleanup-only. > > In general, I'll try to see if we can remove LinkPreload as a type entirely (and > just set it to whatever XHR is). I *think* this priority difference is the only behavioral difference between the LinkPreload and Raw types, so it should be straightforward? > > Also, I'm assuming that you deleted the `setPriority()` as it's no longer > needed, since priority is now set on the resource. Is that correct? s/resource/request/, but otherwise correct. The priority should now only be set directly on the ResourceRequest, by FetchRequest::makeSynchrnous() for sync requests, and by ResourceFetcher for all other cases.
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/20 19:05:03, Nate Chapin wrote: > Sorry for the delay, catching up post-vacation... > > https://codereview.chromium.org/2072613002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): > > https://codereview.chromium.org/2072613002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/LinkLoader.cpp:301: > linkRequest.setDefer(FetchRequest::LazyLoad); > On 2016/06/30 09:09:14, Yoav Weiss wrote: > > I think you left that one out, as we said LinkPreload shouldn't be LazyLoad. > > Also - how are tests passing with this change? :| > > The tests currently expect LinkPreloads to be priority Low, instead of the usual > priority High for XHRs and other Raw resources. ResourceFetcher now > special-cases Raw LazyLoads to get that behavior. If you'd like me to change > what the tests expect instead, I'm happy to do that, but I was aiming to keep > this cleanup-only. > > > > > In general, I'll try to see if we can remove LinkPreload as a type entirely > (and > > just set it to whatever XHR is). > > I *think* this priority difference is the only behavioral difference between the > LinkPreload and Raw types, so it should be straightforward? > > > > > Also, I'm assuming that you deleted the `setPriority()` as it's no longer > > needed, since priority is now set on the resource. Is that correct? > > s/resource/request/, but otherwise correct. The priority should now only be set > directly on the ResourceRequest, by FetchRequest::makeSynchrnous() for sync > requests, and by ResourceFetcher for all other cases. Friendly ping :)
On 2016/07/26 19:29:25, Nate Chapin wrote: > On 2016/07/20 19:05:03, Nate Chapin wrote: > > Sorry for the delay, catching up post-vacation... > > > > > https://codereview.chromium.org/2072613002/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): > > > > > https://codereview.chromium.org/2072613002/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/loader/LinkLoader.cpp:301: > > linkRequest.setDefer(FetchRequest::LazyLoad); > > On 2016/06/30 09:09:14, Yoav Weiss wrote: > > > I think you left that one out, as we said LinkPreload shouldn't be LazyLoad. > > > Also - how are tests passing with this change? :| > > > > The tests currently expect LinkPreloads to be priority Low, instead of the > usual > > priority High for XHRs and other Raw resources. ResourceFetcher now > > special-cases Raw LazyLoads to get that behavior. If you'd like me to change > > what the tests expect instead, I'm happy to do that, but I was aiming to keep > > this cleanup-only. > > > > > > > > In general, I'll try to see if we can remove LinkPreload as a type entirely > > (and > > > just set it to whatever XHR is). > > > > I *think* this priority difference is the only behavioral difference between > the > > LinkPreload and Raw types, so it should be straightforward? > > > > > > > > Also, I'm assuming that you deleted the `setPriority()` as it's no longer > > > needed, since priority is now set on the resource. Is that correct? > > > > s/resource/request/, but otherwise correct. The priority should now only be > set > > directly on the ResourceRequest, by FetchRequest::makeSynchrnous() for sync > > requests, and by ResourceFetcher for all other cases. > > Friendly ping :) LGTM. I'll remove LinkPreload entirely when I get a chance and set other means to recognize such requests as link preloads (in order to eventually support as=document for iframes, so to make sure these resources are cached for short periods of time in the HTTP cache)
The CQ bit was checked by japhet@chromium.org
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by japhet@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 ========== Make ResourceLoadPriority calculation simpler * Don't keep a separate ResourceLoadPriority value on FetchRequest, just use ResourceRequest::m_priority. * Make a bit more use of FetchRequest::LazyLoad for cases like preloads where the priority of a request should be very low. * ResourceFetcher::loadPriority() shouldn't blindly trust the priority given by a caller, and instead takes the max of the provided priority and the priority it calculates. * Add a helper to FetchRequest to make a request synchronous, so callers don't need to manually flag the request as Highest priority. This may or may not fix a case where synchronous requests are incorrectly and flakily being given a priority lower than Highest. BUG=620334 ========== to ========== Make ResourceLoadPriority calculation simpler * Don't keep a separate ResourceLoadPriority value on FetchRequest, just use ResourceRequest::m_priority. * Make a bit more use of FetchRequest::LazyLoad for cases like preloads where the priority of a request should be very low. * ResourceFetcher::loadPriority() shouldn't blindly trust the priority given by a caller, and instead takes the max of the provided priority and the priority it calculates. * Add a helper to FetchRequest to make a request synchronous, so callers don't need to manually flag the request as Highest priority. This may or may not fix a case where synchronous requests are incorrectly and flakily being given a priority lower than Highest. BUG=620334 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Make ResourceLoadPriority calculation simpler * Don't keep a separate ResourceLoadPriority value on FetchRequest, just use ResourceRequest::m_priority. * Make a bit more use of FetchRequest::LazyLoad for cases like preloads where the priority of a request should be very low. * ResourceFetcher::loadPriority() shouldn't blindly trust the priority given by a caller, and instead takes the max of the provided priority and the priority it calculates. * Add a helper to FetchRequest to make a request synchronous, so callers don't need to manually flag the request as Highest priority. This may or may not fix a case where synchronous requests are incorrectly and flakily being given a priority lower than Highest. BUG=620334 ========== to ========== Make ResourceLoadPriority calculation simpler * Don't keep a separate ResourceLoadPriority value on FetchRequest, just use ResourceRequest::m_priority. * Make a bit more use of FetchRequest::LazyLoad for cases like preloads where the priority of a request should be very low. * ResourceFetcher::loadPriority() shouldn't blindly trust the priority given by a caller, and instead takes the max of the provided priority and the priority it calculates. * Add a helper to FetchRequest to make a request synchronous, so callers don't need to manually flag the request as Highest priority. This may or may not fix a case where synchronous requests are incorrectly and flakily being given a priority lower than Highest. BUG=620334 Committed: https://crrev.com/fd0671f01914a4f5e74620e75f673f213d302823 Cr-Commit-Position: refs/heads/master@{#410216} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fd0671f01914a4f5e74620e75f673f213d302823 Cr-Commit-Position: refs/heads/master@{#410216} |
