396222 - let clients track the offline cache usage for a domain

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Dave Camp (:dcamp)]

Attachment: v1

The UI needs this to show the disk space usage for offline apps.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

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)

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v2

Fixed cbiesinger's comments

Comment 3

[Honza Bambas (:mayhemer)]

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.

Comment 4

[Dave Camp (:dcamp)]

hrm yeah, it's probably worth adding that index.

Comment 5

[Honza Bambas (:mayhemer)]

(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.

Comment 6

[Dave Camp (:dcamp)]

Honzab: any updates here?  Can we get the Domain index added so we can get this patch in?

Comment 7

[Honza Bambas (:mayhemer)]

(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.

Comment 8

[Reed Loden [:reed]]

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+.

Comment 9

[Reed Loden [:reed]]

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