Closed Bug 370195 Opened 14 years ago Closed 14 years ago

sql device for the offline cache

Categories

(Core :: Networking: Cache, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dcamp, Assigned: dcamp)

References

Details

Attachments

(1 file, 7 obsolete files)

Attached patch v1 (obsolete) — 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).
Also, this doesn't yet have the securityinfo caching darin mentioned.
Attached patch v2 (obsolete) — Splinter Review
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
Attached patch v3 (obsolete) — Splinter Review
New version;  this one adds some helpers for managing the list of offline resources.
Attachment #256586 - Attachment is obsolete: true
Blocks: 372969
Blocks: 372970
Attached patch v4 (obsolete) — Splinter Review
* 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
Attachment #258742 - Flags: review?(cbiesinger)
Attached patch v4.1 (obsolete) — Splinter Review
(other patch had a rogue unrelated file deletion)
Attachment #258742 - Attachment is obsolete: true
Attachment #258743 - Flags: review?(cbiesinger)
Attachment #258742 - Flags: review?(cbiesinger)
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.
Attachment #258743 - Flags: review?(cbiesinger) → review-
OS: Mac OS X → All
Hardware: PC → All
Attached patch v5 (obsolete) — Splinter Review
Attachment #258743 - Attachment is obsolete: true
Attachment #262560 - Flags: review?(cbiesinger)
(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.
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 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+
Attached patch v6 (obsolete) — Splinter Review
Updated for biesi's comments.
Attachment #262560 - Attachment is obsolete: true
Attachment #264264 - Flags: review?(jst)
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+
Attached patch v7Splinter Review
added a check on NS_NewURI().
Attachment #264264 - Attachment is obsolete: true
checked in.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Blocks: 379590
Flags: in-testsuite?
Blocks: 398161
Depends on: 727579
No longer depends on: 727579
You need to log in before you can comment on or make changes to this bug.