let clients track the offline cache usage for a domain

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
Networking: Cache
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: dcamp, Assigned: dcamp)

Tracking

Trunk
mozilla1.9beta4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
Created attachment 280937 [details] [diff] [review]
v1

The UI needs this to show the disk space usage for offline apps.
Attachment #280937 - Flags: superreview?(cbiesinger)
Attachment #280937 - Flags: review?(cbiesinger)
(Assignee)

Updated

11 years ago
Attachment #280937 - Attachment is patch: true
Attachment #280937 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

11 years ago
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+
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.
(Assignee)

Comment 4

10 years ago
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.
(Assignee)

Comment 6

10 years ago
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.
(Assignee)

Updated

10 years ago
Attachment #295798 - Flags: review?(dcamp) → review+
Keywords: checkin-needed
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?

Updated

10 years ago
Attachment #295798 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
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: 10 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.