Summary: | Use DeferrableOneShotTimer for deleting decoded data | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||
Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, commit-queue, japhet, kling, simon.fraser | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Antti Koivisto
2013-06-27 14:45:26 PDT
Created attachment 205640 [details]
patch
Comment on attachment 205640 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=205640&action=review > Source/WebCore/platform/Timer.h:158 > + void setDelay(double delay) { m_delay = delay; } setDelay() seems ambiguous here; does it reschedule the current timer? What does it mean to just change the delay like this? Created attachment 205648 [details]
patch
Remove the confusing delay setter, instead use a virtual function to vary the delay constructor argument.
Comment on attachment 205648 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=205648&action=review r=me > Source/WebCore/ChangeLog:35 > + Avoid unnecessary restart after stopping and starting again. Nice ninja fix. Comment on attachment 205648 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=205648&action=review > Source/WebCore/loader/cache/CachedResource.cpp:894 > +double CachedResource::decodedDataDeletionTimerDelay() const > +{ > + return memoryCache()->deadDecodedDataDeletionInterval(); > +} Shame that one is called an interval, and the other a delay. Comment on attachment 205648 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=205648&action=review > Source/WebCore/loader/cache/CachedScript.h:60 > + virtual double decodedDataDeletionTimerDelay() const OVERRIDE; Does this do what you mean? You're calling decodedDataDeletionTimerDelay() in the CachedResource construct, which means you'll get the implementation from the base class because the CachedScript constructor hasn't run yet. Good point. It is indeed wrong. Created attachment 205803 [details]
don't call virtual in constructor
Comment on attachment 205803 [details] don't call virtual in constructor View in context: https://bugs.webkit.org/attachment.cgi?id=205803&action=review Oh wow, now I feel stupid. :| > Source/WebCore/loader/cache/CachedResource.cpp:185 > +static double decodedDataDeletionTimerDelayForResourceType(CachedResource::Type type) We should call this deadDecodedDataDeletionIntervalForResourceType() to match the MemoryCache function. |