Closed Bug 372969 Opened 17 years ago Closed 17 years ago

implement navigator.pendingOfflineLoads

Categories

(Core :: DOM: Core & HTML, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: dcamp, Assigned: dcamp)

References

(Depends on 1 open bug, )

Details

Attachments

(1 file, 4 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
Checkpointing my implementation of the proposed navigator.pendingOfflineLoads API described at http://www.campd.org/stuff/Offline%20Cache.html
Blocks: 372970
Attached patch v2 (obsolete) — Splinter Review
This patch does two things:

* Adds navigator.pendingOfflineLoads.  I'm not too sure about the DOMEventTarget implementation on this.

* Rearranges the prefetch service to fire requested/completed events and let clients look at the queue.
Attachment #257598 - Attachment is obsolete: true
Attachment #264774 - Flags: review?(cbiesinger)
Comment on attachment 264774 [details] [diff] [review]
v2

a/uriloader/prefetch/nsIPrefetchService.idl

please use spaces rather than tabs

it would be nice to add your new argument to the comments too

please document the type of the items in the enumerator



b/uriloader/prefetch/nsPrefetchService.cpp
+nsPrefetchQueueEnumerator::GetNext(nsISupports **aItem)
+{
+    if (!mCurrent) return NS_ERROR_NOT_AVAILABLE;

That doesn't match what nsISimpleEnumerator documents...

+NS_IMPL_ISUPPORTS1(nsPrefetchQueueEnumerator, nsISimpleEnumerator);

don't put a trailing semicolon here, that breaks gcc in -pedantic mode


+nsPrefetchNode::ConsumeSegments(nsIInputStream *aInputStream,

Could replace this function with NS_DiscardSegment

+    if (mBytesRead == 0 && aStatus == NS_OK) {
+        // didn't need to read to the cache, but pretend we did

please mention that this is for LOAD_ONLY_IF_MODIFIED

+    if (aNode)
+        *aNode = node;

hm, it's unusual not to addref out parameters. I'd be happier if you followed normal style and addref it here, or alternatively make the node the return value (instead of the current nsresult)

+            if (!aOffline || !mCurrentNode->mOffline) {

Isn't the current logic more like:
  if (!aOffline || mCurrentNode->mOffline) {

? (ie. the second condition is not negated)

+    aURI = NS_ConvertUTF8toUTF16(spec);

CopyUTF8toUTF16(spec, aURI);

+    rv = httpChannel->GetResponseStatus(&httpStatus);
+    if (rv == NS_ERROR_NOT_AVAILABLE) {
+        *aStatus = 0;

Is this supposed to have the same semantics as XMLHttpRequest? Because that does something else in this case...



Personally I'd also use C++-style casts (or for the PRUint16, a constructor-style cast) instead of the C-style ones :)


(I only looked at the uriloader/ changes)
Attachment #264774 - Flags: review?(cbiesinger) → review-
also: I don't see dom/tests/mochitest/ajax/offline/Makefile.in in CVS, is that part of another patch somewhere?
Flags: blocking1.9?
Target Milestone: --- → mozilla1.9alpha5
Flags: blocking1.9? → blocking1.9+
Attached patch v3 (obsolete) — Splinter Review
This version fixes biesi's comments and a few other things.

There's one thing I'm not particularly happy with - from the comment:

nsDOMOfflineLoadStatusList has an array wrapper in its classinfo struct
(LoadStatusList) that exposes nsDOMOfflineLoadStatusList::Item() as
array items.

For scripts to recognize these as nsIDOMLoadStatus objects, I needed
to add a LoadStatus classinfo.

Because the prefetch service is outside the dom module, it can't
actually use the LoadStatus classinfo.

nsDOMOfflineLoadStatus is just a simple wrapper around the
nsIDOMLoadStatus objects returned by the prefetch service that adds the
LoadStatus classinfo implementation.

Is there a better way around this?
Attachment #264774 - Attachment is obsolete: true
Attachment #265760 - Flags: review?(jst)
No longer blocks: 372970
(In reply to comment #3)
> also: I don't see dom/tests/mochitest/ajax/offline/Makefile.in in CVS, is that
> part of another patch somewhere?
> 

Yeah, it depends on the patch in bug 372970.
Depends on: 372670
(In reply to comment #4)
> Is there a better way around this?

There is another way, I'll let you decide if it's better ;-).

Adding the following in nsDocShellModule.cpp:

#define DOCSHELL_DOMCI_EXTENSION_CID   \
{ 0x4e2e38ce, 0x5256, 0x4eaf, \
  { 0xac, 0x2f, 0x2c, 0x34, 0xe8, 0xdf, 0xb1, 0x93 } }
#define DOCSHELL_DOMCI_EXTENSION_CONTRACTID \
"@mozilla.org/docshell-domci-extender;1"

NS_DOMCI_EXTENSION(DocShell)
  NS_DOMCI_EXTENSION_ENTRY_BEGIN(LoadStatus)
    NS_DOMCI_EXTENSION_ENTRY_INTERFACE(nsIDOMLoadStatus)
  NS_DOMCI_EXTENSION_ENTRY_END(LoadStatus, nsIDOMLoadStatus, PR_TRUE, nsnull)
NS_DOMCI_EXTENSION_END

NS_DECL_DOM_CLASSINFO(LoadStatus)

in Initialize(nsIModule* aSelf):

  nsCOMPtr<nsICategoryManager> catman =
    do_GetService(NS_CATEGORYMANAGER_CONTRACTID, &rv);
  NS_ENSURE_SUCCESS(rv, rv);

  nsXPIDLCString previous;
  rv = catman->AddCategoryEntry(JAVASCRIPT_DOM_CLASS,
                                "LoadStatus",
                                DOCSHELL_DOMCI_EXTENSION_CONTRACTID,
                                PR_TRUE, PR_TRUE, getter_Copies(previous));
  NS_ENSURE_SUCCESS(rv, rv);

in Shutdown(nsIModule* aSelf):

  NS_IF_RELEASE(NS_CLASSINFO_NAME(LoadStatus));

in gDocShellModuleInfo:

  { "DocShell DOMCI Extender",
    DOCSHELL_DOMCI_EXTENSION_CID,
    DOCSHELL_DOMCI_EXTENSION_CONTRACTID,
    NS_DOMCI_EXTENSION_CONSTRUCTOR(DocShell)
  }

in nsPrefetchService.cpp expand NS_IMPL_ISUPPORTS5(nsPrefetchNode, ...) and add:

  NS_INTERFACE_MAP_ENTRY_EXTERNAL_DOM_CLASSINFO(LoadStatus)

I think that would make it work without the wrapper.
Not happening for A5, moving to A6.
Target Milestone: mozilla1.9alpha5 → mozilla1.9alpha6
Comment on attachment 265760 [details] [diff] [review]
v3

+++ b/dom/src/offline/nsDOMOfflineLoadStatusList.cpp	Tue May 22 20:34:17 2007 -0700
+    ownerURI = NS_LITERAL_CSTRING("");

ownerURI.Truncate()


I didn't look very closely at the changes outside of uriloader, I'll leave that to jst who knows that code much better
Attachment #265760 - Flags: review?(cbiesinger) → review+
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Attached patch v4 (obsolete) — Splinter Review
This replaces the NS_LITERAL_CSTRING("") with Truncate().  It also defers initialization of the object (as was done in bug 383421)
Attachment #265760 - Attachment is obsolete: true
Attachment #270232 - Flags: superreview?(jst)
Attachment #270232 - Flags: review?(jst)
Attachment #265760 - Flags: review?(jst)
Comment on attachment 270232 [details] [diff] [review]
v4

- In nsDOMOfflineLoadStatusList::Item(PRUint32 aItem, nsIDOMLoadStatus **aStatus):

+  if ((PRInt32)aItem > mItems.Count()) return NS_ERROR_NOT_AVAILABLE;

NS_ERROR_DOM_INDEX_SIZE_ERR?

- In nsPrefetchQueueEnumerator::Increment():

+    do {
+        if (!mCurrent) {
+            mCurrent = mService->GetCurrentNode();
+            if (!mCurrent)
+                mCurrent = mService->GetQueueHead();
+        }
+        else {
+            if (mCurrent == mService->GetCurrentNode())
+                mCurrent = mService->GetQueueHead();
+            else
+                mCurrent = mCurrent->mNext;
+        }
+    } while (mCurrent && mIncludeOffline != mCurrent->mOffline &&
+             mIncludeNormal == mCurrent->mOffline);

This loop needs comments. And couldn't this be an infinite loop too, if we start with !mCurrent, we get the current node, then later on if mCurrent == mService->GetCurrentNode() we go back to the head, and seems like we could then hit the current node again, and go back to the head, etc etc. If that's not the case that should be explained in comments IMO.

r+sr=jst with that fixed.
Attachment #270232 - Flags: superreview?(jst)
Attachment #270232 - Flags: superreview+
Attachment #270232 - Flags: review?(jst)
Attachment #270232 - Flags: review+
Attachment #270232 - Attachment is obsolete: true
(In reply to comment #11)
> This loop needs comments. And couldn't this be an infinite loop too, if we
> start with !mCurrent, we get the current node, then later on if mCurrent ==
> mService->GetCurrentNode() we go back to the head, and seems like we could then
> hit the current node again, and go back to the head, etc etc. If that's not the
> case that should be explained in comments IMO.

The current node has already been removed from the pending queue, so it won't be both in the current node and in the list.  I changed the loop to only check the current node the first time through (rather than testing !mCurrent for that) and added comments explaining what's up.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Er, the patch had tests -- didn't realize that initially.  In the future, Dave, could you make sure to flip the in-testsuite flag to + when you commit a patch with tests?  Thanks!
Flags: in-testsuite? → in-testsuite+
Blocks: 398161
Depends on: 408431
Depends on: 562403
Depends on: 565500
Depends on: 727579
No longer depends on: 727579
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.