implement navigator.offlineResources

RESOLVED FIXED in mozilla1.9alpha5

Status

()

Core
DOM
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: dcamp, Assigned: dcamp)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

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

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

Comment 1

10 years ago
Created attachment 264327 [details] [diff] [review]
v2
Attachment #257599 - Attachment is obsolete: true
Attachment #264327 - Flags: review?(jst)
(Assignee)

Comment 2

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

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

Comment 4

10 years ago
Created attachment 264910 [details]
v4
Attachment #264567 - Attachment is obsolete: true
(Assignee)

Comment 5

10 years ago
> - 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.
(Assignee)

Comment 6

10 years ago
Created attachment 264921 [details] [diff] [review]
v5

updates the prefetch service check on Refresh() too.
Attachment #264910 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #264921 - Flags: superreview?(jst)
(Assignee)

Comment 7

10 years ago
Created attachment 264923 [details] [diff] [review]
v5.1

oops, return rv if mOfflineResources->Init() fails
Attachment #264921 - Attachment is obsolete: true
Attachment #264923 - Flags: superreview?(jst)
Attachment #264921 - Flags: superreview?(jst)
(Assignee)

Comment 8

10 years ago
Created attachment 264924 [details] [diff] [review]
v5.2

attached the wrong patch, sorry :/
Attachment #264923 - Attachment is obsolete: true
Attachment #264924 - Flags: superreview?(jst)
Attachment #264923 - Flags: superreview?(jst)
(Assignee)

Comment 9

10 years ago
Created attachment 264935 [details] [diff] [review]
v6

updates the uuid, deals with a null docshell
Attachment #264924 - Attachment is obsolete: true
Attachment #264935 - Flags: superreview?(jst)
Attachment #264924 - Flags: superreview?(jst)
(Assignee)

Updated

10 years ago
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+

Updated

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

Updated

10 years ago
No longer depends on: 372969
(Assignee)

Updated

10 years ago
Blocks: 373231
Checked in.

Dave, you're ready to get CVS access :-)
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/dom/tests/mochitest/ajax/offline/test_offlineResources.html&root=/cvsroot
Flags: in-testsuite+
This caused bug 382482, which breaks zimbra (and anything else that tries to access navigator.offlineResources.  Makes sad dogfood!

Updated

10 years ago
Depends on: 382482
(Assignee)

Updated

10 years ago
Blocks: 398161

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.