implement navigator.pendingOfflineLoads

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
DOM
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: dcamp, Assigned: dcamp)

Tracking

(Depends on: 1 bug)

unspecified
mozilla1.9alpha8
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 257598 [details] [diff] [review]
v1

Checkpointing my implementation of the proposed navigator.pendingOfflineLoads API described at http://www.campd.org/stuff/Offline%20Cache.html
(Assignee)

Updated

10 years ago
Blocks: 372970
(Assignee)

Comment 1

10 years ago
Created attachment 264774 [details] [diff] [review]
v2

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?
(Assignee)

Updated

10 years ago
Flags: blocking1.9?
Target Milestone: --- → mozilla1.9alpha5

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 4

10 years ago
Created attachment 265760 [details] [diff] [review]
v3

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)
(Assignee)

Updated

10 years ago
No longer blocks: 372970
(Assignee)

Comment 5

10 years ago
(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.
Attachment #265760 - Flags: review?(cbiesinger)
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
(Assignee)

Comment 10

10 years ago
Created attachment 270232 [details] [diff] [review]
v4

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+
(Assignee)

Comment 12

10 years ago
Created attachment 271490 [details] [diff] [review]
patch as committed
Attachment #270232 - Attachment is obsolete: true
(Assignee)

Comment 13

10 years ago
(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
Last Resolved: 10 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+
(Assignee)

Updated

10 years ago
Blocks: 398161

Updated

10 years ago
Depends on: 408431

Updated

7 years ago
Depends on: 562403
Depends on: 565500

Updated

5 years ago
Depends on: 727579

Updated

5 years ago
No longer depends on: 727579
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.