Note nsICacheEntryInfo is, apart from nsAboutCache, only used in about 6 locations to get the ExpirationTime and in 1 location to set the ExpirationTime. May be the expirationTime should become an attribute of nsICachingChannel, so that it can be get/set directly from that, just like Get/SetCacheFromFile? The only real implementor of nsICachingChannel is nsHttpChannel. If ExpirationTime is an attribute to nsICachingChannel, the various callers (except AboutCache), can directly get/set that attribute from the CachingChannel. /browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp, line 306 -- nsCOMPtr<nsICacheEntryInfo> entryInfo = do_QueryInterface(cacheToken); /browser/components/places/src/nsBookmarksFeedHandler.cpp, line 275 -- nsCOMPtr<nsICacheEntryInfo> entryInfo = do_QueryInterface(cacheToken); /docshell/base/nsDocShell.cpp, line 7501 -- nsCOMPtr<nsICacheEntryInfo> cacheEntryInfo(do_QueryInterface(cacheToken)); /modules/libpr0n/src/imgRequest.cpp, line 644 -- nsCOMPtr<nsICacheEntryInfo> entryDesc(do_QueryInterface(cacheToken)); /uriloader/prefetch/nsPrefetchService.cpp, line 166 -- nsCOMPtr<nsICacheEntryInfo> entryInfo(do_QueryInterface(cacheToken, &rv)); /widget/src/mac/nsSound.cpp, line 410 -- nsCOMPtr<nsICacheEntryInfo> cacheEntryInfo = do_QueryInterface(cacheToken); So, before: nsCOMPtr<nsICachingChannel> channel = do_QueryInterface(aRequest); if (channel) { nsCOMPtr<nsISupports> cacheToken; channel->GetCacheToken(getter_AddRefs(cacheToken)); if (cacheToken) { nsCOMPtr<nsICacheEntryInfo> entryInfo = do_QueryInterface(cacheToken); if (entryInfo) { PRUint32 expiresTime; if (NS_SUCCEEDED(entryInfo->GetExpirationTime(&expiresTime))) { .... } } } After: nsCOMPtr<nsICachingChannel> channel = do_QueryInterface(aRequest); if (channel && (NS_SUCCEEDED(channel->GetExpirationTime(&expiresTime)))) { .... } Bug 230675 is about cleaning up the cache visitor / cacheEntryInfo / cacheEntryDescriptor stuff, saving about 47KiB code. This bug should be done first to add Get/SetExpirationTime to nsICachingChannel, making nsICacheEntryInfo redundant (except for in nsAboutCache).

This patch adds GetExpirationTime to nsICachingChannel so that most of the callers of getCacheToken/CacheEntryInfo can now directly do GetExpirationTime. Also made 'cacheToken' an readonly attribute, as the Setter was not implemented anyway.

This patch makes GetExpirationTime a member of nsICachingChannel. Which prevents the need to QI the cachetoken to nsICacheEntryInfo, which in turn prevents the need for nsICacheEntryInfo completely. This saves about 5K (!) per caller in code object size. Also mCacheStream in the bookmark/places code is unused and thus removed. Also the nsICacheVisitor.h include is not needed and is removed.

Comment on attachment 216850 [details] [diff] [review] V2: Also do browser part >Index: netwerk/base/public/nsICachingChannel.idl > [scriptable, uuid(b1f95f5e-ee05-4434-9d34-89a935d7feef)] > interface nsICachingChannel : nsISupports > { > /** >+ * Get the expiration time of the cache entry (in seconds since the Epoch). >+ */ >+ readonly attribute PRUint32 expirationTime; When changing an interface, it is essential that you change the UUID property of the interface. Otherwise, you break binary compatibility with previous versions of Mozilla, which can cause all kinds of havoc for extension authors. Index: browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp >+ if (NS_SUCCEEDED(channel->GetExpirationTime(&expiresTime))) { >+ PRInt64 temp64, nowtime = PR_Now(); >+ PRUint32 nowsec; >+ LL_I2L(temp64, PR_USEC_PER_SEC); >+ LL_DIV(temp64, nowtime, temp64); >+ LL_L2UI(nowsec, temp64); >+ >+ if (nowsec >= expiresTime) { >+ expiresTime -= nowsec; >+ if (ttl < (PRInt32) expiresTime) >+ ttl = (PRInt32) expiresTime; > } There is no need to use the LL_ macros anymore. You can assume that the compiler supports PRInt64/PRUint64 arithmetic. r=darin w/ the UUID change. please post a new patch for check-in.

Note: the TTL calculation is strange: + if (nowsec >= expiresTime) { + expiresTime -= nowsec; + if (ttl < (PRInt32) expiresTime) + ttl = (PRInt32) expiresTime; That means that if nowsec (currenttime) is past the expiresTime, the expiresTime becomes negative (but stored as unsigned so very big!), and the TTL then becomes very big?

Darin, can you take a look at this?

This patch needs to be unbitrotted, but is still valid in concept, simplifying the cache interface.

Is this bug still relevant?

It is still very relevant, as this is DECOM, and general code simplification.

Comment on attachment 219586 [details] [diff] [review] V3: New UUID and replaced LL calculations Probably bitrotted, but still useful in making cache code simpler.

I'm really not a good reviewer for the cache stuff; I'm not that familiar with it. Perhaps Michal? Or maybe Jason can recommend someone?

James, ping for review please - or if you're not the right person to review/don't have the time to review at the moment, please can you tell me who else to ask - thanks! :-)

Comment on attachment 219586 [details] [diff] [review] V3: New UUID and replaced LL calculations I'll let bjarne or michal (or another cache person) take this

Comment on attachment 219586 [details] [diff] [review] V3: New UUID and replaced LL calculations Review of attachment 219586 [details] [diff] [review]: ----------------------------------------------------------------- Patch has bitrotted and e.g. uses PRBool everywhere - please fix. Also, nsBookmarksFeedHandler.cpp does not seem to exist anymore - logic seems to have moved to nsLivemarkService.js. Moreover, I observe that even more cpp-files should be updated with this improvement - you may want to do another search like in comment #0 to find them. I'm giving this an r- because there are a number of small issues plus the patch has bitrotted, but the approach and code is generally fine. Please re-request review when patch has been updated. ::: docshell/base/nsDocShell.cpp @@ +7460,5 @@ > */ > if (cacheChannel) { > cacheChannel->GetCacheKey(getter_AddRefs(cacheKey)); > + PRUint32 expTime; > + cacheChannel->GetExpirationTime(&expTime); Please handle error-returns, or comment / indicate that you don't care about it. Note that |expTime| has no default value. @@ +7496,5 @@ > */ > if (discardLayoutState) { > entry->SetSaveLayoutStateFlag(PR_FALSE); > } > + if (expired == PR_TRUE) { Drop `==` ::: modules/libpr0n/src/imgRequest.cpp @@ +639,5 @@ > nsCOMPtr<nsICachingChannel> cacheChannel(do_QueryInterface(aRequest)); > if (cacheChannel) { > + PRUint32 expiration; > + /* get the expiration time from the caching channel's token */ > + cacheChannel->GetExpirationTime(&expiration); Please handle error-returns here. Note also that all this logic has been moved to the SetCacheValidation() method. ::: widget/src/mac/nsSound.cpp @@ +404,5 @@ > // try to get the expiration time from the URI load > nsCOMPtr<nsICachingChannel> cachingChannel = do_QueryInterface(inChannel); > if (cachingChannel) > { > + cachingChannel->GetExpirationTime(&expirationTime); Please handle error-returns, or comment / indicate that you don't care about it. ::: netwerk/base/public/nsICachingChannel.idl @@ +71,5 @@ > * > * The cache token can be QI'd to a nsICacheEntryInfo if more detail > * about the cache entry is needed (e.g., expiration time). > */ > + readonly attribute nsISupports cacheToken; I don't think this should be done as part of this patch (just register a new bug for it), but if you really want to make it readonly you need to clean up comment. ::: browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp @@ +303,5 @@ > + PRUint32 nowsec = (PRUint32)(PR_Now() / (PRInt64)PR_USEC_PER_SEC); > + if (nowsec >= expiresTime) { > + expiresTime -= nowsec; > + if (ttl < (PRInt32) expiresTime) > + ttl = (PRInt32) expiresTime; I agree with your comment #4 that this looks funny. However, thus stuff seems to have moved to nsLivemarkService.js and logic is different there.

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258