Closed
Bug 370195
Opened 18 years ago
Closed 18 years ago
sql device for the offline cache
Categories
(Core :: Networking: Cache, defect)
Core
Networking: Cache
Tracking
()
RESOLVED
FIXED
People
(Reporter: dcamp, Assigned: dcamp)
References
Details
Attachments
(1 file, 7 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
this patch adapts darin's sql cache device for the offline cache.
Currently it evicts entries that haven't been fetched for a specified number of days (defaults to 60).
Assignee | ||
Comment 1•18 years ago
|
||
Also, this doesn't yet have the securityinfo caching darin mentioned.
Assignee | ||
Comment 2•18 years ago
|
||
This version adds "ownership" to the offline cache device. Unowned entries are evicted. The prefetch service makes sure the ownership is up-to-date for <link rel="offline-resource"> links.
There's still a max size preference (set to 500megs) but it isn't actually implemented.
Attachment #254846 -
Attachment is obsolete: true
Assignee | ||
Comment 3•18 years ago
|
||
New version; this one adds some helpers for managing the list of offline resources.
Attachment #256586 -
Attachment is obsolete: true
Assignee | ||
Comment 4•18 years ago
|
||
* Fixes the build if MOZ_STORAGE isn't defined (by disabling the offline cache build)
* Deals with out-of-space by dooming new entries (under the assumption that a currently-running web app can at least show the user that it wasn't able to sync, rather than an old app being pushed out)
* This version of the patch patches nsDiskCacheDeviceSQL.[h,cpp] rather than moving it to nsOfflineCacheDevice.cpp - after landing it might be nice to rename the file.
Attachment #257594 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Attachment #258742 -
Flags: review?(cbiesinger)
Assignee | ||
Comment 5•18 years ago
|
||
(other patch had a rogue unrelated file deletion)
Attachment #258742 -
Attachment is obsolete: true
Attachment #258743 -
Flags: review?(cbiesinger)
Attachment #258742 -
Flags: review?(cbiesinger)
Comment 6•18 years ago
|
||
Hm, do we really want this to take up 512 MB of users' disks?
netwerk/cache/public/Makefile.in
Your indentation seems inconsistent with the other lines
netwerk/cache/public/nsIOfflineCacheSession.idl
+ Cache
+ * entries owned by a domain will not be purged from the offline cache.
What if the space limit is exceeded?
Hm... why the CStringArrayRef versions?
Also - what happens if the cache doesn't yet have an entry for the given key,
is the call essentially ignored or does it "take effect" as soon as such an
entry is added?
What happens if I attempt to remove a key that doesn't exist?
(please add the answers to these questions to the comments :) )
netwerk/cache/src/Makefile.in
+CPPSRCS += \
+ nsDiskCacheDeviceSQL.cpp
please add a $(NULL) on a new line
netwerk/cache/src/nsCacheService.cpp
+nsresult nsCacheService::SetOfflineOwnedKeys(nsCacheSession * session,
Code style here is to put the "nsresult" on its own line
+ if (session->StoragePolicy() != nsICache::STORE_OFFLINE) return NS_ERROR_NOT_AVAILABLE;
a linebreak would be nice
netwerk/cache/src/nsCacheSession.cpp
+NS_IMPL_ISUPPORTS2(nsCacheSession,
+ nsICacheSession,
+ nsIOfflineCacheSession)
perhaps you should only allow QI to nsIOfflineCacheSession when this is for an
offline session. you can do it with the interface map macros and
NS_INTERFACE_MAP_ENTRY_CONDITIONAL.
please prefer C++ casts over C-style ones, e.g. here:
+ *count = (PRUint32)keyArray.Count();
+ char **ret = (char**)nsMemory::Alloc(sizeof(char*) * (*count));
+ ret[i] = nsCRT::strdup(keyArray[i]->get());
wrong allocator, use NS_strdup (to go with NS_Free. nsCRT::strdup has another
free function)
+ nsMemory::Free((void*)ret);
no need to cast to void*
\ No newline at end of file
please fix :)
netwerk/cache/src/nsDiskCacheDeviceSQL.cpp
+// helper function for directly exposing the same data file binding
+// path algorithm used in nsOfflineCacheBinding::Create
Could nsOfflineCacheBinding::Create be changed to use this function, to avoid
duplicating the algorithm? though... I note that you just moved this function,
so feel free to leave it as-is
+ *aDescription = nsCRT::strdup("Offline cache device");
Hm, this was wrong before... need to use NS_strdup here too (would be great if
you could fix this for all functions here that return strings via xpcom
interfaces)
- do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID, &rv);
+ do_GetService("@mozilla.org/storage/service;1", &rv);
Why this change?
+ rv = mDB->CreateFunction("cache_eviction_observer", -1,
+ new nsOfflineCacheEvictionFunction(this));
Why not pass 3 as the number of arguments?
Also, passing around objects with zero refcount makes me nervous... I'd store
the nsOfflineCacheEvictionFunction in an nsCOMPtr before passing it here
+ rv = binding->mDataFile->IsFile(&isFile);
+ NS_ENSURE_SUCCESS(rv, nsnull);
+ if (!isFile)
Isn't it much more likely that the function call fails than that it returns
false? I.e. isn't it more likely that the file doesn't exist than that it is
(say) a directory?
My point being: I think you should treat failure rv the same as !isFile
+ // mark as active
+ AutoResetStatement updateStatement(mStatement_UpdateEntryFlags);
+ rec.flags &= 0x1;
Shouldn't that be |=?
+nsOfflineCacheDevice::OpenInputStreamForEntry(nsCacheEntry *entry,
nsCacheAccessMode mode,
Please fix the indentation of the parameters here
+ // the entry will overran the cache capacity, doom the entry
+ // and abort
s/overran/overrun/, right?
So once the cache is filled up, no new entries can be added?
+ NS_ASSERTION(NS_SUCCEEDED(rv),"DoomEntry() failed.");
please add a space after the comma
+ nsDependentCString clientIDStr(clientID);
If you move this some lines up, you can use it where you currently have
nsDependentCString(clientID)
(in SetOwnedKeys)
+nsOfflineCacheDevice::SetCapacity(PRUint32 capacity)
+{
+ mCacheCapacity = capacity;
Please put back the * 1024 here :)
uriloader/prefetch/Makefile.in
pref \
+ dom \
+ unicharutil \
Inconsistent indentation here
uriloader/prefetch/nsPrefetchService.cpp
+static NS_DEFINE_IID(kCacheServiceCID, NS_CACHESERVICE_CID);
Well, firstly this should be _CID, but I'd prefer if you used the contract ID
instead
+/* XXX: is there really not a string split function anywhere? Should something
nsCStringArray has one, but I'm not aware of a unicode split function, indeed.
+ nsAutoString subString;
you don't seem to use this variable
But it seems a lot simpler to move this code to the current code in
nsHTMLContentSink/nsContentSink that looks for these <link>s.
Updated•18 years ago
|
Attachment #258743 -
Flags: review?(cbiesinger) → review-
Updated•18 years ago
|
OS: Mac OS X → All
Hardware: PC → All
Assignee | ||
Comment 7•18 years ago
|
||
Attachment #258743 -
Attachment is obsolete: true
Attachment #262560 -
Flags: review?(cbiesinger)
Assignee | ||
Comment 8•18 years ago
|
||
(In reply to comment #6)
Fixed stuff, some specific comments:
> Hm, do we really want this to take up 512 MB of users' disks?
I dropped this to 100 MB until we have the right management UI.
> netwerk/cache/public/nsIOfflineCacheSession.idl
>
> + Cache
> + * entries owned by a domain will not be purged from the offline cache.
>
> What if the space limit is exceeded?
>
> Hm... why the CStringArrayRef versions?
>
> Also - what happens if the cache doesn't yet have an entry for the given key,
> is the call essentially ignored or does it "take effect" as soon as such an
> entry is added?
>
> What happens if I attempt to remove a key that doesn't exist?
>
> (please add the answers to these questions to the comments :) )
I reworked the comments in that file.
> + *aDescription = nsCRT::strdup("Offline cache device");
>
> Hm, this was wrong before... need to use NS_strdup here too (would be great if
> you could fix this for all functions here that return strings via xpcom
> interfaces)
I fixed that up for everything in netwerk/cache/src
> - do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID, &rv);
> + do_GetService("@mozilla.org/storage/service;1", &rv);
>
> Why this change?
So, pulling storage/build/ into the necko tier to get that header would have dragged too much; I suppose I could do it with a LOCAL_INCLUDE if you want.
> uriloader/prefetch/nsPrefetchService.cpp
> +static NS_DEFINE_IID(kCacheServiceCID, NS_CACHESERVICE_CID);
>
> Well, firstly this should be _CID, but I'd prefer if you used the contract ID
> instead
Did that, and switched the other CIDs to use contract ids in that file too.
> But it seems a lot simpler to move this code to the current code in
> nsHTMLContentSink/nsContentSink that looks for these <link>s.
Moved that inot the content sink.
Comment 9•18 years ago
|
||
netwerk/cache/src/nsDiskCacheDeviceSQL.cpp
LOG(("nsOfflineCacheDevice::RemoveOwnedKey [cid=%s]\n", clientID));
[...]
+ if (isOwned) return NS_ERROR_NOT_AVAILABLE;
Shouldn't that be !isOwned?
uriloader/prefetch/nsPrefetchService.cpp
+ do_GetService(NS_CACHESERVICE_CONTRACTID,
+ &rv);
I think this'd look nicer w/o this linebreak
I think you don't need the makefile changes in uriloader anymore, or the nsIDOM* headers
content/base/src/nsContentSink.cpp
+ if (FindInReadable(NS_LITERAL_CSTRING("#"), specStart, specEnd)) {
Should probably use FindCharInReadable here
However, I'm not a content/ peer, so a content peer will have to review that part too.
r=biesi on everything except content, sr=biesi on content
Comment 10•18 years ago
|
||
Comment on attachment 262560 [details] [diff] [review]
v5
Oh... forgot to mention:
in nsContentSink.cpp:
+ } else
+ offlineCacheSession->AddOwnedKey(ownerHost, ownerSpec, spec);
content/ style is to add {} even for single-line if bodies.
Attachment #262560 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Comment 11•18 years ago
|
||
Updated for biesi's comments.
Attachment #262560 -
Attachment is obsolete: true
Attachment #264264 -
Flags: review?(jst)
Comment 12•18 years ago
|
||
Comment on attachment 264264 [details] [diff] [review]
v6
- In nsContentSink::AddOfflineResource(const nsAString &aHref):
+ nsCOMPtr<nsIURI> uri;
+ NS_NewURI(getter_AddRefs(uri), aHref,
+ charset.IsEmpty() ? nsnull : PromiseFlatCString(charset).get(),
+ mDocumentBaseURI);
+
+ // only http and https urls can be marked as offline resources
+ rv = uri->SchemeIs("http", &match);
Check that the NS_NewURI() call succeeded before you dereference the uri you created.
Other than that the content changes look good to me.
Attachment #264264 -
Flags: review?(jst) → review+
Assignee | ||
Comment 13•18 years ago
|
||
added a check on NS_NewURI().
Attachment #264264 -
Attachment is obsolete: true
Comment 14•18 years ago
|
||
checked in.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 15•18 years ago
|
||
Did this perhaps cause: https://bugzilla.mozilla.org/show_bug.cgi?id=380741
Updated•18 years ago
|
Flags: in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•