Closed Bug 388839 Opened 13 years ago Closed 13 years ago

object to manage updates to the offline cache

Categories

(Core :: Networking: Cache, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dcamp, Assigned: dcamp)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
This patch moves the logic for updating the offline cache and adding to the owners list in to an nsOfflineCacheUpdate object.  It updates the content sink and navigator.offlineResources to use this object instead of the prefetch service, and navigator.pendingOfflineLoads to monitor it instead of the prefetch service.
Attachment #273009 - Flags: review?(cbiesinger)
Comment on attachment 273009 [details] [diff] [review]
v1

nsIOfflineCacheSession needs a new IID 

uriloader/prefetch/nsIOfflineCacheUpdate.idl
+[scriptable, uuid(7dc06ede-1098-4384-b95e-65525ab254c9)]
+interface nsIOfflineCacheUpdate : nsISupports {
+  /**
+   * An nsIOfflineCacheUpdate is used to update a domain's offline resources.
+   * It can be used to perform partial or complete updates.

You should put that comment above the interface, that way tools like
doxygen can find it.

+   * @param aDocument Schedule the update when this document finished loading.

That doesn't sound like the right comment here

+  readonly attribute unsigned long numItems;
+  nsIDOMLoadStatus item(in unsigned long index); 

Perhaps numItems would better be named count? (for consistency with
nsIDOMNodeList, which also has an item() function)

uriloader/prefetch/nsOfflineCacheUpdate.cpp
Shouldn't the AutoFreeArray data members be private?

nsOfflineCacheUpdateItem::OnStartRequest doesn't need the QI anymore 

as mentioned on irc, nsOfflineCacheUpdateItem::Cancel should still reset
mState.

nsOfflineCacheUpdateItem::OnChannelRedirect doesn't use oldCachingChannel

+    *aSource = nsnull; 
+    nsCOMPtr<nsIDOMNode> source = do_QueryReferent(mSource);
+    if (source)
+        source.swap(*aSource);
+
+    return NS_OK;

return CallQueryReferent(mSource, aSource);

Note that your ItemCompleted calls on the observers don't handle observers
that remove themselves, is that a problem? perhaps you should copy that
list and iterate on the copy.

You should probably also remove weak references for which the object has
gone away at some point.

+    mDocUpdates.Init();

Should check whether that failed

+            NS_RELEASE(gOfflineCacheUpdateService);
+            gOfflineCacheUpdateService = nsnull; 

NS_RELEASE already takes care of the null setting


What if NewChannel or AsyncOpen fails for a certain URL? ProcessNextUpdate
will have set mUpdateRunning to true already. It seems like no further
updates would ever run, right?

+        if (mDocUpdates.Count() < 0)
+            return NS_OK;

Hm, when would that be less than zero? Do you mean <= ?
Attachment #273009 - Flags: review?(cbiesinger) → review-
Attached patch v2 (obsolete) — Splinter Review
Assignee: nobody → dcamp
Attachment #273009 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #273083 - Flags: superreview?(jst)
Attachment #273083 - Flags: review?(cbiesinger)
Attachment #273083 - Flags: review?(cbiesinger) → review+
Comment on attachment 273083 [details] [diff] [review]
v2

actually I meant to comment on the doxygen comment style too; for comments like:

+   * @param aPartialUpdate TRUE if the update should just update the URIs
+   *        given to it, FALSE if all URLs for the owner domain
+   *        should be added.

I think it would be nicer to write them as:
  * @param aPartialUpdate
           TRUE if the [...]
Comment on attachment 273083 [details] [diff] [review]
v2

- In uriloader/prefetch/nsIOfflineCacheUpdate.idl:

+  /**
+   * Add a URI to the offline cache as part of the update.
+   *
+   * @param aURI The URI to add.
+   * @param aURI The DOM node (<link> tag) associated with this node (for
+   *        implementing nsIDOMLoadStatus).
+   */
+  void addURI(in nsIURI aURI, in nsIDOMNode aSource);

The second @param names the wrong argument.

sr=jst
Attachment #273083 - Flags: superreview?(jst) → superreview+
Attachment #273083 - Attachment is obsolete: true
Checking in content/base/src/nsContentSink.cpp;
/cvsroot/mozilla/content/base/src/nsContentSink.cpp,v  <--  nsContentSink.cpp
new revision: 1.86; previous revision: 1.85
done
Checking in content/base/src/nsContentSink.h;
/cvsroot/mozilla/content/base/src/nsContentSink.h,v  <--  nsContentSink.h
new revision: 1.39; previous revision: 1.38
done
Checking in content/html/document/src/nsHTMLContentSink.cpp;
/cvsroot/mozilla/content/html/document/src/nsHTMLContentSink.cpp,v  <--  nsHTMLContentSink.cpp
new revision: 3.795; previous revision: 3.794
done
Checking in docshell/build/Makefile.in;
/cvsroot/mozilla/docshell/build/Makefile.in,v  <--  Makefile.in
new revision: 1.47; previous revision: 1.46
done
Checking in docshell/build/nsDocShellModule.cpp;
/cvsroot/mozilla/docshell/build/nsDocShellModule.cpp,v  <--  nsDocShellModule.cpp
new revision: 1.29; previous revision: 1.28
done
Checking in dom/src/base/Makefile.in;
/cvsroot/mozilla/dom/src/base/Makefile.in,v  <--  Makefile.in
new revision: 1.81; previous revision: 1.80
done
Checking in dom/src/offline/nsDOMOfflineLoadStatusList.cpp;
/cvsroot/mozilla/dom/src/offline/nsDOMOfflineLoadStatusList.cpp,v  <--  nsDOMOfflineLoadStatusList.cpp
new revision: 1.2; previous revision: 1.1
done
Checking in dom/src/offline/nsDOMOfflineLoadStatusList.h;
/cvsroot/mozilla/dom/src/offline/nsDOMOfflineLoadStatusList.h,v  <--  nsDOMOfflineLoadStatusList.h
new revision: 1.2; previous revision: 1.1
done
Checking in dom/src/offline/nsDOMOfflineResourceList.cpp;
/cvsroot/mozilla/dom/src/offline/nsDOMOfflineResourceList.cpp,v  <--  nsDOMOfflineResourceList.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in dom/tests/mochitest/ajax/offline/test_pendingOfflineLoads.html;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_pendingOfflineLoads.html,v  <--  test_pendingOfflineLoads.html
new revision: 1.2; previous revision: 1.1
done
Checking in netwerk/cache/public/nsIOfflineCacheSession.idl;
/cvsroot/mozilla/netwerk/cache/public/nsIOfflineCacheSession.idl,v  <--  nsIOfflineCacheSession.idl
new revision: 1.2; previous revision: 1.1
done
Checking in netwerk/cache/src/nsCacheService.cpp;
/cvsroot/mozilla/netwerk/cache/src/nsCacheService.cpp,v  <--  nsCacheService.cpp
new revision: 1.113; previous revision: 1.112
done
Checking in netwerk/cache/src/nsCacheService.h;
/cvsroot/mozilla/netwerk/cache/src/nsCacheService.h,v  <--  nsCacheService.h
new revision: 1.49; previous revision: 1.48
done
Checking in netwerk/cache/src/nsCacheSession.cpp;
/cvsroot/mozilla/netwerk/cache/src/nsCacheSession.cpp,v  <--  nsCacheSession.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in netwerk/cache/src/nsDiskCacheDeviceSQL.cpp;
/cvsroot/mozilla/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp,v  <--  nsDiskCacheDeviceSQL.cpp
new revision: 1.13; previous revision: 1.12
done
Checking in netwerk/cache/src/nsDiskCacheDeviceSQL.h;
/cvsroot/mozilla/netwerk/cache/src/nsDiskCacheDeviceSQL.h,v  <--  nsDiskCacheDeviceSQL.h
new revision: 1.5; previous revision: 1.4
done
Checking in uriloader/prefetch/Makefile.in;
/cvsroot/mozilla/uriloader/prefetch/Makefile.in,v  <--  Makefile.in
new revision: 1.6; previous revision: 1.5
done
Checking in uriloader/prefetch/nsCPrefetchService.h;
/cvsroot/mozilla/uriloader/prefetch/nsCPrefetchService.h,v  <--  nsCPrefetchService.h
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/uriloader/prefetch/nsIOfflineCacheUpdate.idl,v
done
Checking in uriloader/prefetch/nsIOfflineCacheUpdate.idl;
/cvsroot/mozilla/uriloader/prefetch/nsIOfflineCacheUpdate.idl,v  <--  nsIOfflineCacheUpdate.idl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.cpp,v
done
Checking in uriloader/prefetch/nsOfflineCacheUpdate.cpp;
/cvsroot/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.cpp,v  <--  nsOfflineCacheUpdate.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.h,v
done
Checking in uriloader/prefetch/nsOfflineCacheUpdate.h;
/cvsroot/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.h,v  <--  nsOfflineCacheUpdate.h
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attached patch bustage fixSplinter Review
this patch was needed on linux
Comment on attachment 273735 [details] [diff] [review]
patch as committed

Doesn't anyone read compiler warnings any more?

>+NS_IMETHODIMP
>+nsOfflineCacheUpdateItem::GetStatus(PRUint16 *aStatus)
...
>+            *aStatus = NS_ERROR_NOT_AVAILABLE;
You can't fit a quart into a pint pot.
Comment on attachment 273735 [details] [diff] [review]
patch as committed

>+    for (PRInt32 i = 0; i < mObservers.Count(); i++) {
>+        if (mObservers[i] == aObserver) {
>+            mObservers.RemoveObjectAt(i);
>+            return NS_OK;
>+        }
>+    }
mObservers.RemoveObject(aObserver); also works.
Blocks: 398161
You need to log in before you can comment on or make changes to this bug.