Closed Bug 396222 Opened 12 years ago Closed 12 years ago

let clients track the offline cache usage for a domain

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: dcamp, Assigned: dcamp)

References

Details

Attachments

(2 files)

Attached patch v1Splinter Review
The UI needs this to show the disk space usage for offline apps.
Attachment #280937 - Flags: superreview?(cbiesinger)
Attachment #280937 - Flags: review?(cbiesinger)
Attachment #280937 - Attachment is patch: true
Attachment #280937 - Attachment mime type: application/octet-stream → text/plain
Blocks: 397417
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)
Attachment #280937 - Flags: superreview?(cbiesinger)
Attachment #280937 - Flags: superreview+
Attachment #280937 - Flags: review?(cbiesinger)
Attachment #280937 - Flags: review+
Attached patch v2Splinter Review
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
Keywords: checkin-needed
Version: unspecified → Trunk
Attachment #295798 - Flags: approval1.9?
Attachment #295798 - Flags: approval1.9? → approval1.9+
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
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
You need to log in before you can comment on or make changes to this bug.