WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
49320
Minor ResourceLoadScheduler cleanups
https://bugs.webkit.org/show_bug.cgi?id=49320
Summary
Minor ResourceLoadScheduler cleanups
Antti Koivisto
Reported
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
Attachments
patch
(6.82 KB, patch)
2010-11-10 07:35 PST
,
Antti Koivisto
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1
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?
Antti Koivisto
Comment 2
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.
Alexey Proskuryakov
Comment 3
2010-11-10 10:29:05 PST
Thanks for explanation! So it sounds like this could have a regression test.
Alexey Proskuryakov
Comment 4
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).
Antti Koivisto
Comment 5
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug