move offline cache APIs toward the whatwg spec

RESOLVED FIXED in mozilla1.9beta3

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: dcamp, Assigned: dcamp)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9beta3
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

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

The attached patch removes our offline cache APIs, replacing them with something closer to what has been specified in the webapps spec.

The major changes are:
* Replace <link rel="offline-resource"> elements with a manifest file that is read and dealt with in nsOfflineCacheUpdate.
* Move the scriptable resource list from navigator.offlineResources to window.applicationCache.
* Move the update events from navigator.offlineResources to window.applicationCache.
* Along the way, make sure the applicationCache object deals correctly with windows that go away and with windows that are frozen in the bfcache.

I reused ns[I]DOMOfflineResourceList to implement applicationCache.  That should probably be renamed to ns[I]DOMApplicationCache, but I didn't want to dirty up the diff with a bunch of renames.  I can do that as a followup.

This is not a complete implementation of the whatwg spec.  In particular, we are missing versioned, independent application caches and we don't serve from the offline cache unless the browser is explicitly offline.
Flags: blocking1.9?
Attachment #287152 - Flags: superreview?(jst)
Attachment #287152 - Flags: review?(jst)
(Assignee)

Comment 1

10 years ago
Created attachment 287153 [details] [diff] [review]
tests

... and some tests.
Attachment #287152 - Attachment is obsolete: true
Attachment #287153 - Flags: review?(jst)
Attachment #287152 - Flags: superreview?(jst)
Attachment #287152 - Flags: review?(jst)
(Assignee)

Updated

10 years ago
Attachment #287152 - Attachment is obsolete: false
Attachment #287152 - Flags: superreview?(jst)
Attachment #287152 - Flags: review?(jst)
(Assignee)

Updated

10 years ago
Attachment #287152 - Flags: review?(cbiesinger)

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Duplicate of this bug: 394352
Comment on attachment 287152 [details] [diff] [review]
v1

- In dom/public/idl/base/nsIDOMClientInformation.idl:

-
-  readonly attribute nsIDOMOfflineResourceList offlineResources;
-  readonly attribute nsIDOMLoadStatusList pendingOfflineLoads;

Do we need to bump the IID here too? Or maybe this never shipped anywhere yet?

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

+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsDOMOfflineResourceList)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mManifestURI)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mDocumentURI)

I don't think any URI objects participate in cycle collection, so no real reason to traverse them, or to unlink them in unlink either.

- In nsDOMOfflineResourceList::SetOnchecking() etc:

+  mScriptContext = GetCurrentContext();

This is somewhat concerning, could lead to inconsistent behavior depending on order etc when multiple windows are accessing the same offline resource list. Given that the nsDOMOfflineResourceList knows its window, I'd rather see this code get the context from the window object when it needs it.

r+sr=jst with that.
Attachment #287152 - Flags: superreview?(jst)
Attachment #287152 - Flags: superreview+
Attachment #287152 - Flags: review?(jst)
Attachment #287152 - Flags: review+

Updated

10 years ago
Attachment #287153 - Flags: review?(jst) → review+
(Assignee)

Comment 4

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

updated with jst's changes.  biesi, can you take a look at the nsOfflineCacheUpdate changes please?
Attachment #287152 - Attachment is obsolete: true
Attachment #290458 - Flags: review?(cbiesinger)
Attachment #287152 - Flags: review?(cbiesinger)
(Assignee)

Updated

10 years ago
Blocks: 405693
(Assignee)

Comment 5

10 years ago
Filed bug 405693 about the XXX in nsOfflineCacheManifestItem::OnStopRequest (shouldn't update a byte-for-byte identical cache manifest)
(Assignee)

Updated

10 years ago
Blocks: 405695
Whiteboard: [has patch][has r/sr jst][needs sr biesi]
(Assignee)

Updated

10 years ago
Blocks: 397416
(Assignee)

Comment 6

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

A few small fixes to v2:

* The ERROR enum value broke on vc8, so I namespaced the parser states.
* Properly fail the update if HandleManifest() indicates that an update is unnecessary.
Attachment #290458 - Attachment is obsolete: true
Attachment #295171 - Flags: review?(cbiesinger)
Attachment #290458 - Flags: review?(cbiesinger)
(Assignee)

Comment 7

10 years ago
Created attachment 295172 [details] [diff] [review]
tests v2

updated the tests to use ^headers^ instead of modifying httpd.js
Attachment #287153 - Attachment is obsolete: true
Comment on attachment 295171 [details] [diff] [review]
v3


+NS_METHOD
+nsOfflineManifestItem::ReadManifest(nsIInputStream *aInputStream,

please add a /* static */ comment to make it clearer that it's a static
member function.

+    manifest->mReadBuf.Append(aFromSegment + aOffset, aCount);

That's not right... see the docs in nsIInputStream. You should start reading
at aFromSegment without adding an offset.

+            nsresult rv = manifest->HandleManifestLine(begin, iter);
+            NS_ENSURE_SUCCESS(rv, rv);

Hm... so if that fails you keep processing the same line over and over again,
for each OnDataAvailable call? Perhaps ODA should check that bytesRead ==
aCount and otherwise return an error.

(note: necko requires that onDataAvailable either reads all the data or
returns an error (or cancels the request). returning an error from the
readSegments callback does not consume the data)

+        if (!(magic == NS_LITERAL_CSTRING("CACHE MANIFEST"))) {

if (!magic.EqualsLiteral("CACHE MANIFEST"))

Similar for other such uses in this function

+nsOfflineManifestItem::OnStartRequest(nsIRequest *aRequest,

I would note that this doesn't check for 404 (or similar), but the MIME type
check should be enough. (Otherwise, you could call
nsIHttpChannel.requestSucceeded)

+    nsresult rv = HandleManifestLine(begin, end);
(in OnStopRequest)

So, here end points beyond the string, so it's not valid to read
from it. However, HandleManifestLine does read from it. That's not good.

+        manifestSpec.SetLength(ref);

Should be .Truncate(ref) I think.

+    if (!mManifestItem) return NS_ERROR_OUT_OF_MEMORY;

please put the return on its own line, so that it's possible to set a
breakpoint on it.
Attachment #295171 - Flags: review?(cbiesinger) → review+
Created attachment 296216 [details] [diff] [review]
v4

(In reply to comment #8)
> (From update of attachment 295171 [details] [diff] [review])
> 
> +NS_METHOD
> +nsOfflineManifestItem::ReadManifest(nsIInputStream *aInputStream,
> 
> please add a /* static */ comment to make it clearer that it's a static
> member function.
> 

done, also on other places

> +    manifest->mReadBuf.Append(aFromSegment + aOffset, aCount);
> 
> That's not right... see the docs in nsIInputStream. You should start reading
> at aFromSegment without adding an offset.

fixed as suggested and tested

> 
> +            nsresult rv = manifest->HandleManifestLine(begin, iter);
> +            NS_ENSURE_SUCCESS(rv, rv);
> 
> Hm... so if that fails you keep processing the same line over and over again,
> for each OnDataAvailable call? Perhaps ODA should check that bytesRead ==
> aCount and otherwise return an error.
> 
> (note: necko requires that onDataAvailable either reads all the data or
> returns an error (or cancels the request). returning an error from the
> readSegments callback does not consume the data)
> 

I set mParserState to PARSER_ERROR and exit reader with NS_ERROR_ABORT, OnDataAvailable then check the parser state and return NS_ERROR_ABORT in case of PARSE_ERROR

> +        if (!(magic == NS_LITERAL_CSTRING("CACHE MANIFEST"))) {
> 
> if (!magic.EqualsLiteral("CACHE MANIFEST"))
> 
> Similar for other such uses in this function
> 

done

> +nsOfflineManifestItem::OnStartRequest(nsIRequest *aRequest,
> 
> I would note that this doesn't check for 404 (or similar), but the MIME type
> check should be enough. (Otherwise, you could call
> nsIHttpChannel.requestSucceeded)
> 

nsIHttpChannel.requestSucceeded added

> +    nsresult rv = HandleManifestLine(begin, end);
> (in OnStopRequest)
> 
> So, here end points beyond the string, so it's not valid to read
> from it. However, HandleManifestLine does read from it. That's not good.
> 

I fixed HandleManifestLine to not to read from the *end position. There were bug in code clearing the trailing spaces and tabs (were not removed because *end points at trailing '\0' char).

> +        manifestSpec.SetLength(ref);
> 
> Should be .Truncate(ref) I think.
> 

done

> +    if (!mManifestItem) return NS_ERROR_OUT_OF_MEMORY;
> 
> please put the return on its own line, so that it's possible to set a
> breakpoint on it.
> 

done
Attachment #295171 - Attachment is obsolete: true
Attachment #296216 - Flags: review?(cbiesinger)
(Assignee)

Comment 10

10 years ago
Created attachment 297417 [details] [diff] [review]
patch as committed

I just committed the attached patch, which contains honzab's fixes and a change I discussed with biesi on irc (including the nsIOfflineCacheUpdate object in nsIOfflineCacheUpdateObserver methods).

Checking in content/base/public/nsContentUtils.h;
/cvsroot/mozilla/content/base/public/nsContentUtils.h,v  <--  nsContentUtils.h
new revision: 1.156; previous revision: 1.155
done
Checking in content/base/src/nsContentSink.cpp;
/cvsroot/mozilla/content/base/src/nsContentSink.cpp,v  <--  nsContentSink.cpp
new revision: 1.87; previous revision: 1.86
done
Checking in content/base/src/nsContentSink.h;
/cvsroot/mozilla/content/base/src/nsContentSink.h,v  <--  nsContentSink.h
new revision: 1.40; previous revision: 1.39
done
Checking in content/base/src/nsContentUtils.cpp;
/cvsroot/mozilla/content/base/src/nsContentUtils.cpp,v  <--  nsContentUtils.cpp
new revision: 1.267; previous revision: 1.266
done
Checking in content/base/src/nsGkAtomList.h;
/cvsroot/mozilla/content/base/src/nsGkAtomList.h,v  <--  nsGkAtomList.h
new revision: 3.90; previous revision: 3.89
done
Checking in content/html/document/src/nsHTMLContentSink.cpp;
/cvsroot/mozilla/content/html/document/src/nsHTMLContentSink.cpp,v  <--  nsHTMLContentSink.cpp
new revision: 3.800; previous revision: 3.799
done
Checking in dom/public/base/nsPIDOMWindow.h;
/cvsroot/mozilla/dom/public/base/nsPIDOMWindow.h,v  <--  nsPIDOMWindow.h
new revision: 1.70; previous revision: 1.69
done
Checking in dom/public/idl/base/nsIDOMClientInformation.idl;
/cvsroot/mozilla/dom/public/idl/base/nsIDOMClientInformation.idl,v  <--  nsIDOMClientInformation.idl
new revision: 1.5; previous revision: 1.4
done
Checking in dom/public/idl/base/nsIDOMWindow2.idl;
/cvsroot/mozilla/dom/public/idl/base/nsIDOMWindow2.idl,v  <--  nsIDOMWindow2.idl
new revision: 1.2; previous revision: 1.1
done
Checking in dom/public/idl/offline/nsIDOMOfflineResourceList.idl;
/cvsroot/mozilla/dom/public/idl/offline/nsIDOMOfflineResourceList.idl,v  <--  nsIDOMOfflineResourceList.idl
new revision: 1.2; previous revision: 1.1
done
Checking in dom/src/base/nsGlobalWindow.cpp;
/cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v  <--  nsGlobalWindow.cpp
new revision: 1.973; previous revision: 1.972
done
Checking in dom/src/base/nsGlobalWindow.h;
/cvsroot/mozilla/dom/src/base/nsGlobalWindow.h,v  <--  nsGlobalWindow.h
new revision: 1.320; previous revision: 1.319
done
Checking in dom/src/offline/Makefile.in;
/cvsroot/mozilla/dom/src/offline/Makefile.in,v  <--  Makefile.in
new revision: 1.4; previous revision: 1.3
done
Removing dom/src/offline/nsDOMOfflineLoadStatusList.h;
/cvsroot/mozilla/dom/src/offline/nsDOMOfflineLoadStatusList.h,v  <--  nsDOMOfflineLoadStatusList.h
new revision: delete; previous revision: 1.3
done
Checking in dom/src/offline/nsDOMOfflineResourceList.cpp;
/cvsroot/mozilla/dom/src/offline/nsDOMOfflineResourceList.cpp,v  <--  nsDOMOfflineResourceList.cpp
new revision: 1.5; previous revision: 1.4
done
Checking in dom/src/offline/nsDOMOfflineResourceList.h;
/cvsroot/mozilla/dom/src/offline/nsDOMOfflineResourceList.h,v  <--  nsDOMOfflineResourceList.h
new revision: 1.3; previous revision: 1.2
done
Checking in dom/tests/mochitest/ajax/offline/Makefile.in;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/Makefile.in,v  <--  Makefile.in
new revision: 1.6; previous revision: 1.5
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/badManifestMagic.cacheManifest,v
done
Checking in dom/tests/mochitest/ajax/offline/badManifestMagic.cacheManifest;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/badManifestMagic.cacheManifest,v  <--  badManifestMagic.cacheManifest
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/badManifestMagic.cacheManifest^headers^,v
done
Checking in dom/tests/mochitest/ajax/offline/badManifestMagic.cacheManifest^headers^;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/badManifestMagic.cacheManifest^headers^,v  <--  badManifestMagic.cacheManifest^headers^
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/missingFile.cacheManifest,v
done
Checking in dom/tests/mochitest/ajax/offline/missingFile.cacheManifest;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/missingFile.cacheManifest,v  <--  missingFile.cacheManifest
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/missingFile.cacheManifest^headers^,v
done
Checking in dom/tests/mochitest/ajax/offline/missingFile.cacheManifest^headers^;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/missingFile.cacheManifest^headers^,v  <--  missingFile.cacheManifest^headers^
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/offlineChild.html,v
done
Checking in dom/tests/mochitest/ajax/offline/offlineChild.html;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/offlineChild.html,v  <--  offlineChild.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/offlineTests.js,v
done
Checking in dom/tests/mochitest/ajax/offline/offlineTests.js;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/offlineTests.js,v  <--  offlineTests.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/simpleManifest.cacheManifest,v
done
Checking in dom/tests/mochitest/ajax/offline/simpleManifest.cacheManifest;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/simpleManifest.cacheManifest,v  <--  simpleManifest.cacheManifest
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/simpleManifest.cacheManifest^headers^,v
done
Checking in dom/tests/mochitest/ajax/offline/simpleManifest.cacheManifest^headers^;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/simpleManifest.cacheManifest^headers^,v  <--  simpleManifest.cacheManifest^headers^
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/simpleManifest.notmanifest,v
done
Checking in dom/tests/mochitest/ajax/offline/simpleManifest.notmanifest;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/simpleManifest.notmanifest,v  <--  simpleManifest.notmanifest
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_badManifestMagic.html,v
done
Checking in dom/tests/mochitest/ajax/offline/test_badManifestMagic.html;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_badManifestMagic.html,v  <--  test_badManifestMagic.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_badManifestMime.html,v
done
Checking in dom/tests/mochitest/ajax/offline/test_badManifestMime.html;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_badManifestMime.html,v  <--  test_badManifestMime.html
initial revision: 1.1
done
Removing dom/tests/mochitest/ajax/offline/test_isLocallyAvailable.html;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_isLocallyAvailable.html,v  <--  test_isLocallyAvailable.html
new revision: delete; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_missingFile.html,v
done
Checking in dom/tests/mochitest/ajax/offline/test_missingFile.html;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_missingFile.html,v  <--  test_missingFile.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_offlineIFrame.html,v
done
Checking in dom/tests/mochitest/ajax/offline/test_offlineIFrame.html;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_offlineIFrame.html,v  <--  test_offlineIFrame.html
initial revision: 1.1
done
Removing dom/tests/mochitest/ajax/offline/test_offlineResources.html;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_offlineResources.html,v  <--  test_offlineResources.html
new revision: delete; previous revision: 1.1
done
Removing dom/tests/mochitest/ajax/offline/test_pendingOfflineLoads.html;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_pendingOfflineLoads.html,v  <--  test_pendingOfflineLoads.html
new revision: delete; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_simpleManifest.html,v
done
Checking in dom/tests/mochitest/ajax/offline/test_simpleManifest.html;
/cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_simpleManifest.html,v  <--  test_simpleManifest.html
initial revision: 1.1
done
Checking in uriloader/prefetch/nsIOfflineCacheUpdate.idl;
/cvsroot/mozilla/uriloader/prefetch/nsIOfflineCacheUpdate.idl,v  <--  nsIOfflineCacheUpdate.idl
new revision: 1.2; previous revision: 1.1
done
Checking in uriloader/prefetch/nsOfflineCacheUpdate.cpp;
/cvsroot/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.cpp,v  <--  nsOfflineCacheUpdate.cpp
new revision: 1.5; previous revision: 1.4
done
Checking in uriloader/prefetch/nsOfflineCacheUpdate.h;
/cvsroot/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.h,v  <--  nsOfflineCacheUpdate.h
new revision: 1.4; previous revision: 1.3
done
Attachment #295172 - Attachment is obsolete: true
Attachment #296216 - Attachment is obsolete: true
Attachment #296216 - Flags: review?(cbiesinger)
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Comment on attachment 297417 [details] [diff] [review]
patch as committed

Just a note that this patch is probably not the one that got into CVS. It for example doesn't contain my fix for the trailing spaces remove in  nsOfflineManifestItem::HandleManifestLine.
It seems like the patch that fixed this had tests, so I'm marking this in-testsuite+.
Or are more tests needed for this bug? Then please set the in-testsuite flag back to ?
Flags: in-testsuite+
Whiteboard: [has patch][has r/sr jst][needs sr biesi]
Target Milestone: --- → mozilla1.9 M11
Version: unspecified → Trunk

Updated

10 years ago
Keywords: dev-doc-needed
This is now documented:

http://developer.mozilla.org/en/docs/Offline_resources_in_Firefox
http://developer.mozilla.org/en/docs/nsIDOMOfflineResourceList
Keywords: dev-doc-needed → dev-doc-complete

Updated

8 years ago
Depends on: 491137
Blocks: 615923
No longer blocks: 615923
Depends on: 615923
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.