Closed Bug 372970 Opened 17 years ago Closed 17 years ago

implement navigator.offlineResources

Categories

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

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha5

People

(Reporter: dcamp, Assigned: dcamp)

References

()

Details

Attachments

(1 file, 7 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
Checkpointing my implementation of the proposed navigator.offlineResources API described at http://www.campd.org/stuff/Offline%20Cache.html
Attachment #257599 - Attachment is patch: true
Attachment #257599 - Attachment mime type: application/octet-stream → text/plain
Attached patch v2 (obsolete) — Splinter Review
Attachment #257599 - Attachment is obsolete: true
Attachment #264327 - Flags: review?(jst)
Attached patch v3 (obsolete) — Splinter Review
This version uses the innermost uri when deciding the owner domain.
Attachment #264327 - Attachment is obsolete: true
Attachment #264567 - Flags: review?(jst)
Attachment #264327 - Flags: review?(jst)
Comment on attachment 264567 [details] [diff] [review]
v3

- In dom/public/idl/offline/nsIDOMOfflineResourceList.idl:

+  void refresh();
+};
\ No newline at end of file

Add a newline.

- In nsNavigator::GetOfflineResources():

+    nsresult rv = nsContentUtils::GetSecurityManager()->
+      GetSubjectPrincipal(getter_AddRefs(principal));
+
+    if (NS_FAILED(rv) || !principal) return NS_ERROR_FAILURE;
+
+    nsCOMPtr<nsIURI> uri;
+    rv = principal->GetURI(getter_AddRefs(uri));
+    if (NS_FAILED(rv) || !uri) return NS_ERROR_FAILURE;
+
+    nsDOMOfflineResourceList *offlineResources;
+    offlineResources = new nsDOMOfflineResourceList();
+    if (!offlineResources) return NS_ERROR_OUT_OF_MEMORY;
+
+    rv = offlineResources->Init(uri);
...

This'll use the caller's URI and not that of where the navigator object came from, and I think we want the latter. The cases where it makes a difference is if chrome or other elevated privilege script reaches out to another windows navigator.offlineResources. nsNavigator has a GetDocShell() method that you can use for this (though it may return null if someone uses a navigator object from an old window).

+    if (NS_FAILED(rv)) {
+      delete offlineResources;
+      return rv;
+    }

You could use nsRefPtr for offlineResource to avoid having to do this manually.

- In dom/src/offline/Makefile.in:

+REQUIRES       = xpcom         \
+		 string        \
+		 content       \
+		 caps          \
+		 gfx           \
+		 js            \
+		 layout        \
+		 locale        \
+		 necko         \
+		 nkcache       \
+		 pref          \
+		 prefetch      \
+		 unicharutil   \
+		 widget        \
+		 xpconnect     \
+		 $(NULL)

That seems like a lot of dependencies for this code. Do we really need all those? Is there a #include that pulls in tons of stuff that requires all those, or can we trim down that list?

- In dom/src/offline/nsDOMOfflineResourceList.cpp:

+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* ***** BEGIN LICENSE BLOCK *****
+* Version: MPL 1.1/GPL 2.0/LGPL 2.1
+*

This doesn't look like a direct copy of the boilerplate licenses (leading spaces missing). See http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c. Formatting matters here since these things are sometimes tweaked in the thousands by scripts etc, so keep it as close as possible.

Same thing in nsDOMOfflineResourceList.h.

- In nsDOMOfflineResourceList::Item():

+  aURI = NS_ConvertUTF8toUTF16(gCachedKeys[aIndex]);

Using CopyUTF8toUTF16() would save you a string copy here.

- In nsDOMOfflineResourceList::Add():

+  // try to start fetching it now, but it's not fatal if it fails
+  nsCOMPtr<nsIPrefetchService> prefetchService =
+    do_GetService(NS_PREFETCHSERVICE_CONTRACTID);
+  if (!prefetchService) return NS_OK;

Wouldn't you still want to let the caller know if we can't find a prefetch service? That pretty much means that no prefetching will happen, ever.

r=jst with that.
Attachment #264567 - Flags: review?(jst) → review+
Attached file v4 (obsolete) —
Attachment #264567 - Attachment is obsolete: true
> - In dom/src/offline/Makefile.in:
> 
> +REQUIRES       = xpcom         \
> +                string        \
...
> +                widget        \
> +                xpconnect     \
> +                $(NULL)
> 
> That seems like a lot of dependencies for this code. Do we really need all
> those? Is there a #include that pulls in tons of stuff that requires all those,
> or can we trim down that list?

nsDOMClassInfo.h ends up pulling a lot of that in;  I was able to get rid of gfx, locale, and unicharutil.

Fixed the other stuff.
Attached patch v5 (obsolete) — Splinter Review
updates the prefetch service check on Refresh() too.
Attachment #264910 - Attachment is obsolete: true
Attachment #264921 - Flags: superreview?(jst)
Attached patch v5.1 (obsolete) — Splinter Review
oops, return rv if mOfflineResources->Init() fails
Attachment #264921 - Attachment is obsolete: true
Attachment #264923 - Flags: superreview?(jst)
Attachment #264921 - Flags: superreview?(jst)
Attached patch v5.2 (obsolete) — Splinter Review
attached the wrong patch, sorry :/
Attachment #264923 - Attachment is obsolete: true
Attachment #264924 - Flags: superreview?(jst)
Attachment #264923 - Flags: superreview?(jst)
Attached patch v6Splinter Review
updates the uuid, deals with a null docshell
Attachment #264924 - Attachment is obsolete: true
Attachment #264935 - Flags: superreview?(jst)
Attachment #264924 - Flags: superreview?(jst)
Flags: blocking1.9?
Target Milestone: --- → mozilla1.9alpha5
Comment on attachment 264935 [details] [diff] [review]
v6

r+sr=jst
Attachment #264935 - Flags: superreview?(jst)
Attachment #264935 - Flags: superreview+
Attachment #264935 - Flags: review+
Flags: blocking1.9? → blocking1.9+
No longer depends on: 372969
Blocks: 373231
Checked in.

Dave, you're ready to get CVS access :-)
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
This caused bug 382482, which breaks zimbra (and anything else that tries to access navigator.offlineResources.  Makes sad dogfood!
Depends on: 382482
Blocks: 398161
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.