Created attachment 280937 [details] [diff] [review] v1 The UI needs this to show the disk space usage for offline apps.
Comment on attachment 280937 [details] [diff] [review] v1 nsIOfflineCacheSession.idl needs a new IID + StatementSql ( mStatement_DomainSize, "SELECT Sum(moz_cache.DataSize) from moz_cache, moz_cache_owners WHERE moz_cache.ClientID=moz_cache_owners.ClientID AND moz_cache.Key = moz_cache_owners.Key AND moz_cache.ClientID=? AND moz_cache_owners.Domain=?;" ), would be nice to use an INNER JOIN here + rv |= statement->BindUTF8StringParameter(1, domain); + NS_ENSURE_SUCCESS(rv, rv); you really shouldn't return rv when two rv's have been or'd together... the value doesn't make sense to the caller then. + NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && hasRows, rv); I think this would be more readable if you tested hasRows separately. I'm also not sure that no rows should cause a warning message on the console (which you'd get with the NS_ENSURE_TRUE)
Created attachment 295798 [details] [diff] [review] v2 Fixed cbiesinger's comments
Attachment #295798 - Flags: review?(dcamp)
Just a note, isn't there missing an index on at least moz_cache_owners.Domain column? If there is lots of rows then the select may be slow because moz_cache_owners table is searched in sequential way.
hrm yeah, it's probably worth adding that index.
(In reply to comment #4) > hrm yeah, it's probably worth adding that index. > As seen from the list of prepared statements searching in that table is done quit often using all of its columns. Because the table seems to be filled with a lot of rows and it is probably not often updated then it IMO makes sense to put some kinf of indexes on all columns. The right decision depends on the data distribution in the table and tests using scripts for query statements analyzes. We have such tools in AllPeers, I am adding our sql master as CC.
Honzab: any updates here? Can we get the Domain index added so we can get this patch in?
(In reply to comment #6) > Honzab: any updates here? Can we get the Domain index added so we can get this > patch in? > Adding that single index would not help in performance. We have no testing/real data to see if there is need to add several indexes on the columns. For now I would commit the patch as is and if there are significant performance problems that we have to find the right way of indexing.
Attachment #295798 - Flags: review?(dcamp) → review+
This can't land without approval. Please do not add the checkin-needed keyword until the patch has approval1.9+ or unless the bug is blocking1.9+/blocking-firefox3+.
Status: NEW → ASSIGNED
Version: unspecified → Trunk
Checking in netwerk/cache/public/nsIOfflineCacheSession.idl; /cvsroot/mozilla/netwerk/cache/public/nsIOfflineCacheSession.idl,v <-- nsIOfflineCacheSession.idl new revision: 1.5; previous revision: 1.4 done Checking in netwerk/cache/src/nsCacheService.cpp; /cvsroot/mozilla/netwerk/cache/src/nsCacheService.cpp,v <-- nsCacheService.cpp new revision: 1.117; previous revision: 1.116 done Checking in netwerk/cache/src/nsCacheService.h; /cvsroot/mozilla/netwerk/cache/src/nsCacheService.h,v <-- nsCacheService.h new revision: 1.51; previous revision: 1.50 done Checking in netwerk/cache/src/nsCacheSession.cpp; /cvsroot/mozilla/netwerk/cache/src/nsCacheSession.cpp,v <-- nsCacheSession.cpp new revision: 1.18; previous revision: 1.17 done Checking in netwerk/cache/src/nsDiskCacheDeviceSQL.cpp; /cvsroot/mozilla/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp,v <-- nsDiskCacheDeviceSQL.cpp new revision: 1.16; previous revision: 1.15 done Checking in netwerk/cache/src/nsDiskCacheDeviceSQL.h; /cvsroot/mozilla/netwerk/cache/src/nsDiskCacheDeviceSQL.h,v <-- nsDiskCacheDeviceSQL.h new revision: 1.8; previous revision: 1.7 done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
You need to log in before you can comment on or make changes to this bug.