Created attachment 73498 [details] patch Minor ResourceLoadScheduler cleanups: - remove assertLoaderBeingCounted() as it is wrong (doesn't take deferred resources into account) and not very useful - add HighestPriority, LowestPriority synonyms so new priorities can be added without modifying the iteration code
Comment on attachment 73498 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=73498&action=review > WebCore/ChangeLog:6 > + - remove assertLoaderBeingCounted() as it is wrong (doesn't take deferred resources into account) and not very useful If it can wrongfully fire, we probably need a regression test. But I don't understand what's wrong about it in practice. It's only called when starting the load, can it actually be affected by defers?
(In reply to comment #1) > (From update of attachment 73498 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73498&action=review > > > WebCore/ChangeLog:6 > > + - remove assertLoaderBeingCounted() as it is wrong (doesn't take deferred resources into account) and not very useful > > If it can wrongfully fire, we probably need a regression test. But I don't understand what's wrong about it in practice. > > It's only called when starting the load, can it actually be affected by defers? There can be deferred higher priority resources pending (perhaps from another document) as the logic in servePendingRequests skips those. The claim that the first resource in the highest priority queue is always loaded first doesn't hold. I hit it loading wsj.com and checked the state in debugger. I think the correct logic for this gets overly complicated for an assert and will be fragile against any future changes. This is only ever invoked from servePendingRequests (through ResourceLoader::start()) and basically checks that some state matches what servePendingRequests makes it at that point. I think it is unlikely to catch anything interesting. Rather than correcting it I think it is better to simply remove it.
Thanks for explanation! So it sounds like this could have a regression test.
Comment on attachment 73498 [details] patch Marking r- per bug 49351 comment 9 (changes in bug 49351 make this assertion correct, and it helped find an actual problem).
Landed the priority enum cleanup part of the patch in http://trac.webkit.org/changeset/72171 The assert was already removed elsewhere.