Bug 49320 - Minor ResourceLoadScheduler cleanups
Summary: Minor ResourceLoadScheduler cleanups
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-10 07:35 PST by Antti Koivisto
Modified: 2010-11-16 20:53 PST (History)
2 users (show)

See Also:


Attachments
patch (6.82 KB, patch)
2010-11-10 07:35 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2010-11-10 07:35:33 PST
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 1 Alexey Proskuryakov 2010-11-10 08:51:28 PST
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?
Comment 2 Antti Koivisto 2010-11-10 10:24:54 PST
(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.
Comment 3 Alexey Proskuryakov 2010-11-10 10:29:05 PST
Thanks for explanation! So it sounds like this could have a regression test.
Comment 4 Alexey Proskuryakov 2010-11-11 11:20:00 PST
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).
Comment 5 Antti Koivisto 2010-11-16 20:53:43 PST
Landed the priority enum cleanup part of the patch in http://trac.webkit.org/changeset/72171

The assert was already removed elsewhere.