Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
462813002:
Changed resource scheduler to block until all critical resources actually load. (Closed)
|
Created:
6 years, 4 months ago by Pat Meenan Modified:
6 years, 4 months ago Reviewers:
mmenke CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionThere are basically 2 phases to the loading of resources. The
"critical" phase is when the resources referenced in the head are
being loaded and then the "regular" phase follows. We limit resources
being loaded during the critical phase because nothing can be rendered
on the screen until they have all loaded.
Historically this was done by tracking when the main HTML parser
reached the <body> tag but that doesn't always work because css
resources do not block the parser.
This patch flags all high priority requests that are started before
the parser reaches the body tag as critical and only exits the
critical loading phase once they have all finished loading.
BUG=402891
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291055
Patch Set 1 #Patch Set 2 : Updated the scheduler logic comment to match the new logic. #
Total comments: 2
Patch Set 3 : Switched to an enum for tracking critical and delayable resources #Patch Set 4 : Updated existing tests and added a new one #
Total comments: 11
Patch Set 5 : Fixed reprioritization in redirect and made DCHECK more paranoid #Patch Set 6 : Changed "critical" to "layout-blocking" #Patch Set 7 : Fixed tracking of pending layout-blocking requests #
Total comments: 15
Patch Set 8 : Cleanup from review feedback #
Total comments: 4
Patch Set 9 : Cleaned up the request classification tracking logic #
Total comments: 6
Patch Set 10 : Changed naming to be clearer #Patch Set 11 : Fixed member initialization order (clang warning) #
Messages
Total messages: 45 (0 generated)
mmenke@ PTAL when you get a chance.
On 2014/08/12 17:13:24, Pat Meenan wrote: > mmenke@ PTAL when you get a chance. I expect to get to this tomorrow (I have several others in the queue, but should have time for this). We should have unit tests for this behavior.
Oops, meant to publish a quick question. https://codereview.chromium.org/462813002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/462813002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:388: SetRequestCritical(request, IsCriticalRequest(request)); Should we have an enum instead of bools?
Switched the separate bools to an enum since the delayable and critical states are non-overlapping. Unit tests coming tomorrow so probably not worth looking until I get those sorted out. It actually broke a bunch of the existing unit tests (just found where they were) because they were built to only start requests but not complete them. The existing tests will actually measure the new behavior well, just need to tweak one of them to have different expectations and test before and after completing the critical request. https://codereview.chromium.org/462813002/diff/20001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/462813002/diff/20001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:388: SetRequestCritical(request, IsCriticalRequest(request)); On 2014/08/12 17:18:37, mmenke wrote: > Should we have an enum instead of bools? Done.
ok, should be ready now. The existing unit tests were updated to handle the fact that the critical requests need to complete to move on to the body phase and a new test was added to make sure it doesn't exit the critical phase until the critical requests complete.
https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:453: DCHECK_LE(total_critical_count_, in_flight_requests_.size()); Maybe these would get more coverage if at the end of SetRequestCategory instead? Could even make these more robust, by actually counting the requests in a category. i.e. DCHECK_EQ(CountRequestsInCategory(DELAYABLE_REQUEST), total_delayable_count_); and then another one for critical requests as well. The accounting here isn't that complicated, but I'm paranoid about it regressing. https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:483: return CRITICAL_REQUEST; Do critical requests prevent has_body_ from becoming true? If not, we'll make them non-critical once has_body_ is true, and they're redirected, since we re-evaluating their priority on redirect. https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:531: // * High-priority requests initiated before the renderer has a <body>. I think it's kinda weird (And very non-intuitive) that "critical" requests can have lower priority than "high priority" requests. This can theoretically have implications for when we start loading delayable requests as well. https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:538: // * Non-delayable, High-priority and SDPY capable requests are issued nit: While you're here, SDPY -> SPDY https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:623: total_critical_count_ != 0 && Shouldn't we check if the current request is critical somewhere? Or am I missing something?
One other question - are you planning on just unconditionally enabling this for everyone, without a field trial? I admit to being very skeptical of relying completely on automated tests from WebPageReplay for figuring out if this actually improves things in the field. That having been said, UMA's PLT metrics haven't exactly proven terribly useful.
FWIW, the tests were WebPagetest (not replay) though not sure if that's what you ultimately meant. Yes, the plan was to enable without a field trial. Pretty much every scheduler adjustment and PLT optimization we have made over the last 2-3 years or so has not used field trials as a gate because of how volatile the UMAs are (and the lack of any good ones for render perf). That said, we do usually keep a pretty close eye on them to make sure nothing unexpected happens (like significantly wide swings). https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:453: DCHECK_LE(total_critical_count_, in_flight_requests_.size()); On 2014/08/13 17:55:11, mmenke wrote: > Maybe these would get more coverage if at the end of SetRequestCategory instead? > Could even make these more robust, by actually counting the requests in a > category. i.e. > > DCHECK_EQ(CountRequestsInCategory(DELAYABLE_REQUEST), total_delayable_count_); > and then another one for critical requests as well. > > The accounting here isn't that complicated, but I'm paranoid about it > regressing. Done. https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:483: return CRITICAL_REQUEST; On 2014/08/13 17:55:11, mmenke wrote: > Do critical requests prevent has_body_ from becoming true? If not, we'll make > them non-critical once has_body_ is true, and they're redirected, since we > re-evaluating their priority on redirect. No, they do not - good catch. I changed the logic to not reset existing critical requests as long as the request priority hasn't dropped to make it a low (can't think of a case where someone would do that but better safe than sorry). https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:531: // * High-priority requests initiated before the renderer has a <body>. On 2014/08/13 17:55:11, mmenke wrote: > I think it's kinda weird (And very non-intuitive) that "critical" requests can > have lower priority than "high priority" requests. This can theoretically have > implications for when we start loading delayable requests as well. Would it be less worrying if I called them render-blocking instead of critical? We don't start loading delayable requests until all of the critical requests have finished (outside of the one-at-a-time gate we allow when we are in the critical phase). All high-priority requests started before we hit the body are critical. I guess it's theoretically possible for the renderer to give a medium priority to head resources and then something in the body gets a higher priority which would cause it to compete with the per-host and connection limits against the critical request but it won't break anything. That feels like logic that the renderer can decide to be smarter about if it really wants to though - I'm not sure we want to have the extra logic on the net side of the fence. https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:538: // * Non-delayable, High-priority and SDPY capable requests are issued On 2014/08/13 17:55:11, mmenke wrote: > nit: While you're here, SDPY -> SPDY Done. https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:623: total_critical_count_ != 0 && On 2014/08/13 17:55:11, mmenke wrote: > Shouldn't we check if the current request is critical somewhere? Or am I > missing something? I added a comment to the priority >= net::LOW check above which is where critical requests would get scheduled (basically always requested as long as they meet the host limit).
> https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... > content/browser/loader/resource_scheduler.cc:623: total_critical_count_ != 0 && > On 2014/08/13 17:55:11, mmenke wrote: > > Shouldn't we check if the current request is critical somewhere? Or am I > > missing something? > > I added a comment to the priority >= net::LOW check above which is where > critical requests would get scheduled (basically always requested as long as > they meet the host limit). Matt: All of the requests that are critical have high priority, but not all high priority requests are critical. From my understanding, he isn't making lower priority requests count as critical, so he doesn't need to add a check that the current resource is critical. All critical resources will load at the same times they currently do. The change is making sure all of the important requests are actually finished before more than one low priority request is sent at a time. Hence the change to check if all critical resources are loaded instead of whether the body has finished.
On 2014/08/13 21:03:19, aiolos wrote: > > > https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... > > content/browser/loader/resource_scheduler.cc:623: total_critical_count_ != 0 > && > > On 2014/08/13 17:55:11, mmenke wrote: > > > Shouldn't we check if the current request is critical somewhere? Or am I > > > missing something? > > > > I added a comment to the priority >= net::LOW check above which is where > > critical requests would get scheduled (basically always requested as long as > > they meet the host limit). > > Matt: All of the requests that are critical have high priority, but not all high > priority requests are critical. From my understanding, he isn't making lower > priority requests count as critical, so he doesn't need to add a check that the > current resource is critical. All critical resources will load at the same times > they currently do. The change is making sure all of the important requests are > actually finished before more than one low priority request is sent at a time. > Hence the change to check if all critical resources are loaded instead of > whether the body has finished. He's marking requests with priority >= net::LOW as critical. It's it's possible that before the body has finished, we get request with a priority of net::LOW, and mark it critical. Then the body finishes, and we get a request with a priority of net::HIGHEST...And it's not marked as critical, but is still given priority over the critical resource, which seems weird to me.
Would calling it "render blocking" instead of critical make it less confusing? FWIW, we consider all requests with priority >= LOW that are requested before the body as "high-priority/non-delayable) and that part of the logic hasn't changed (and even before you could get medium priority requests in the body that were higher priority than the "high-priority" LOW requests. All the patch is supposed to do is move the point from when we move out of the restricted (render-blocking) loading phase to make sure all of the requests we started while in the phase actually complete (minus any delayable requests we loaded opportunistically). I believe all of the other issues have been addressed.
On 2014/08/14 12:13:52, Pat Meenan wrote: > Would calling it "render blocking" instead of critical make it less confusing? > FWIW, we consider all requests with priority >= LOW that are requested before > the body as "high-priority/non-delayable) and that part of the logic hasn't > changed (and even before you could get medium priority requests in the body that > were higher priority than the "high-priority" LOW requests. > > All the patch is supposed to do is move the point from when we move out of the > restricted (render-blocking) loading phase to make sure all of the requests we > started while in the phase actually complete (minus any delayable requests we > loaded opportunistically). > > I believe all of the other issues have been addressed. I like RENDER_BLOCKING or PAINT_BLOCKING (LAYOUT_BLOCKING?) request. I'll do a full pass later today - may be fairly late in the day, as I have a lot of meetings today, and a couple other reviews.
I changed all of the "critical" language to "layout-blocking" (enums, variables, comments, etc) so it should be a lot less confusing.
Actually, hold off for today. The current tracking of the layout_blocking requests is only tracking the ones that are in-flight and it should be tracking them at the time they are scheduled (even if they go into the pending queue). Otherwise layout-blocking requests that are delayed because of too many other requests have a chance to not be flagged correctly if we hit the body before the backlog has been cleared. Cleaning up the logic around that right now.
ok, I fixed it to track all layout-blocking requests (in-flight and pending) and I fixed the variable name tracking the delayable requests to make it clear that it is only tracking in-flight requests. No rush on the review, whenever you find yourself doing reviews and have time.
Quick comments. https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/462813002/diff/60001/content/browser/loader/r... content/browser/loader/resource_scheduler.cc:623: total_critical_count_ != 0 && On 2014/08/13 19:24:55, Pat Meenan wrote: > On 2014/08/13 17:55:11, mmenke wrote: > > Shouldn't we check if the current request is critical somewhere? Or am I > > missing something? > > I added a comment to the priority >= net::LOW check above which is where > critical requests would get scheduled (basically always requested as long as > they meet the host limit). Oops - forgot to delete that comment once I figured out what was going on. https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:265: // validate the counts is accurate. nit: Don't use we in comments, since it's ambiguous (x2). https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:267: SetRequestCategory(request, LAYOUT_BLOCKING_REQUEST); Think SetRequestCategory(GetRequestCategory()) is a little weird. Maybe Set->Compute or Calculate or somesuch? https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:267: SetRequestCategory(request, LAYOUT_BLOCKING_REQUEST); I think it's weird that layout blocking requests are layout blocking for life, but delayable requests have a category() of delayable only once they've been started, despite having a GetRequestCategory of delayable. Having trouble coming up with a clean solution to this - I understand this is for accounting purposes, but it results in a confusing API. https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:468: bool include_pending) { nit: const https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:511: // category across redirects unless we chose to lower the priority of the nit: Don't use we in comments. https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:515: return LAYOUT_BLOCKING_REQUEST; nit: Use braces when the conditional takes up more than one line. https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:656: in_flight_requests_.size() > in_flight_delayable_count_; Random comment, no need to worry about in this CL: SPDY requests count towards in_flight_requests_.size here. Should they? Seems weird that lower priority SPDY requests jump to the front of the queue, potentially delaying the start of low-priority non-SPDY requests.
https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:265: // validate the counts is accurate. On 2014/08/15 19:53:40, mmenke wrote: > nit: Don't use we in comments, since it's ambiguous (x2). Done. https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:267: SetRequestCategory(request, LAYOUT_BLOCKING_REQUEST); On 2014/08/15 19:53:40, mmenke wrote: > I think it's weird that layout blocking requests are layout blocking for life, > but delayable requests have a category() of delayable only once they've been > started, despite having a GetRequestCategory of delayable. > > Having trouble coming up with a clean solution to this - I understand this is > for accounting purposes, but it results in a confusing API. Agreed. I could set the category on everything all of the time but then we'd have to move the accounting out and that would get distributed again (and it's logic would be just about as confusing because it would be doing separate accounting depending on if the request is in-flight or not). I'm good with doing it either way but I think this might be simpler even though it's still confusing. https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:468: bool include_pending) { On 2014/08/15 19:53:40, mmenke wrote: > nit: const Done. https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:511: // category across redirects unless we chose to lower the priority of the On 2014/08/15 19:53:40, mmenke wrote: > nit: Don't use we in comments. Done. https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:515: return LAYOUT_BLOCKING_REQUEST; On 2014/08/15 19:53:40, mmenke wrote: > nit: Use braces when the conditional takes up more than one line. Done. https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:656: in_flight_requests_.size() > in_flight_delayable_count_; On 2014/08/15 19:53:40, mmenke wrote: > Random comment, no need to worry about in this CL: SPDY requests count towards > in_flight_requests_.size here. Should they? Seems weird that lower priority > SPDY requests jump to the front of the queue, potentially delaying the start of > low-priority non-SPDY requests. SPDY handling across multiple connections in general is pretty weak (and by weak I mean non-existent). We just dump all requests into a SPDY connection regardless of state or anything else and expect the server to handle prioritization but that only works when you're only talking to a single server which isn't the case for just about any site I've ever looked at, including Google's. It's probably worth re-visiting that and applying the same multi-phase logic across SPDY connections as well (probably don't want to take it further than that).
https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:267: SetRequestCategory(request, LAYOUT_BLOCKING_REQUEST); On 2014/08/15 21:39:31, Pat Meenan wrote: > On 2014/08/15 19:53:40, mmenke wrote: > > I think it's weird that layout blocking requests are layout blocking for life, > > but delayable requests have a category() of delayable only once they've been > > started, despite having a GetRequestCategory of delayable. > > > > Having trouble coming up with a clean solution to this - I understand this is > > for accounting purposes, but it results in a confusing API. > > Agreed. I could set the category on everything all of the time but then we'd > have to move the accounting out and that would get distributed again (and it's > logic would be just about as confusing because it would be doing separate > accounting depending on if the request is in-flight or not). I'm good with > doing it either way but I think this might be simpler even though it's still > confusing. I'm on the fence... Clarity reduces the change of regression, and the other approach is clearer... But it's also more complicated, which increases the chance of regressions. If we keep the current behavior, however, we should have comments about this weirdness (Probably in a couple places - above the category_ itself, in SetRequestCategory, and in the code that only sets the LAYOUT_BLOCKING category), and SetRequestCategory should have a DCHECK that when we set a request as delayable, it's in in_flight_requests_. https://codereview.chromium.org/462813002/diff/140001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/462813002/diff/140001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:554: // Requests are categorized into five categories: Think it's worth noting these are not mutually exclusive categories. For a throttle renderer, layout-blocking SPDY requests are treated different from layout-blocking non-SPDY requests, and non-layout-blocking SPDY requests, for instance. https://codereview.chromium.org/462813002/diff/140001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:576: // requests. This seems redundant with the two you added/modified below. Suggest just getting rid of it, and maybe updating the "once all..." line to include the case there are no layout-blocking requests.
https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/462813002/diff/120001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:267: SetRequestCategory(request, LAYOUT_BLOCKING_REQUEST); On 2014/08/18 16:30:20, mmenke wrote: > On 2014/08/15 21:39:31, Pat Meenan wrote: > > On 2014/08/15 19:53:40, mmenke wrote: > > > I think it's weird that layout blocking requests are layout blocking for > life, > > > but delayable requests have a category() of delayable only once they've been > > > started, despite having a GetRequestCategory of delayable. > > > > > > Having trouble coming up with a clean solution to this - I understand this > is > > > for accounting purposes, but it results in a confusing API. > > > > Agreed. I could set the category on everything all of the time but then we'd > > have to move the accounting out and that would get distributed again (and it's > > logic would be just about as confusing because it would be doing separate > > accounting depending on if the request is in-flight or not). I'm good with > > doing it either way but I think this might be simpler even though it's still > > confusing. > > I'm on the fence... Clarity reduces the change of regression, and the other > approach is clearer... But it's also more complicated, which increases the > chance of regressions. > > If we keep the current behavior, however, we should have comments about this > weirdness (Probably in a couple places - above the category_ itself, in > SetRequestCategory, and in the code that only sets the LAYOUT_BLOCKING > category), and SetRequestCategory should have a DCHECK that when we set a > request as delayable, it's in in_flight_requests_. I think I found a cleaner way to solve it. I changed the delayable designation to in-flight delayable, always set the category for every request and check to see if it is in-flight at the point where it gets set. I also changed "category" to "classification" because it seems closer to how we actually treat the requests. All of the logic is contained in the set/get classification calls and we just need to make sure it is called when requests are scheduled, priorities changed or started (all of which are already covered). Do you think we need comments about making sure to call SetRequestClassification() at all of these points explicitly? https://codereview.chromium.org/462813002/diff/140001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/462813002/diff/140001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:554: // Requests are categorized into five categories: On 2014/08/18 16:30:20, mmenke wrote: > Think it's worth noting these are not mutually exclusive categories. For a > throttle renderer, layout-blocking SPDY requests are treated different from > layout-blocking non-SPDY requests, and non-layout-blocking SPDY requests, for > instance. Done. (changed the language to say it evaluates requests based on 5 attributes). https://codereview.chromium.org/462813002/diff/140001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:576: // requests. On 2014/08/18 16:30:20, mmenke wrote: > This seems redundant with the two you added/modified below. Suggest just > getting rid of it, and maybe updating the "once all..." line to include the case > there are no layout-blocking requests. Done.
This looks pretty reasonable to me. https://codereview.chromium.org/462813002/diff/160001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/462813002/diff/160001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:485: bool in_flight = !pending_requests_.IsQueued(request); This isn't used. https://codereview.chromium.org/462813002/diff/160001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:505: RequestClassification GetRequestClassification( Still think we should reconsider this name, to make it clear at callsites that this is not request->classification(). https://codereview.chromium.org/462813002/diff/160001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:523: !pending_requests_.IsQueued(request)) { Can we check in_flight_requests_ instead? Seems weird to check it's not in the pending list, rather than whether it is in the in flight request, particularly since we call it IN_FLIGHT_DELAYABLE_REQUEST.
https://codereview.chromium.org/462813002/diff/160001/content/browser/loader/... File content/browser/loader/resource_scheduler.cc (right): https://codereview.chromium.org/462813002/diff/160001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:485: bool in_flight = !pending_requests_.IsQueued(request); On 2014/08/18 20:59:43, mmenke wrote: > This isn't used. Done. https://codereview.chromium.org/462813002/diff/160001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:505: RequestClassification GetRequestClassification( On 2014/08/18 20:59:43, mmenke wrote: > Still think we should reconsider this name, to make it clear at callsites that > this is not request->classification(). Done. Changed it to ClassifyRequest() which better reflects what it actually does. https://codereview.chromium.org/462813002/diff/160001/content/browser/loader/... content/browser/loader/resource_scheduler.cc:523: !pending_requests_.IsQueued(request)) { On 2014/08/18 20:59:42, mmenke wrote: > Can we check in_flight_requests_ instead? Seems weird to check it's not in the > pending list, rather than whether it is in the in flight request, particularly > since we call it IN_FLIGHT_DELAYABLE_REQUEST. Done.
LGTM!
The CQ bit was checked by pmeenan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pmeenan@chromium.org/462813002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/44166) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pmeenan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pmeenan@chromium.org/462813002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/44212) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pmeenan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pmeenan@chromium.org/462813002/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was checked by pmeenan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pmeenan@chromium.org/462813002/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was checked by pmeenan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pmeenan@chromium.org/462813002/200001
Message was sent while issue was closed.
Committed patchset #11 (200001) as 291055 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
