Bug 49320 – Minor ResourceLoadScheduler cleanups

• Component: Page Loading
• Status: RESOLVED FIXED

[Antti Koivisto]

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

[Alexey Proskuryakov]

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?

[Antti Koivisto]

(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.

[Alexey Proskuryakov]

Thanks for explanation! So it sounds like this could have a regression test.

[Alexey Proskuryakov]

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).

[Antti Koivisto]

Landed the priority enum cleanup part of the patch in http://trac.webkit.org/changeset/72171

The assert was already removed elsewhere.