Closed Bug 402272 Opened 12 years ago Closed 12 years ago

move offline cache APIs toward the whatwg spec

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: dcamp, Assigned: dcamp)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 6 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
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)
Attached patch tests (obsolete) — Splinter Review
... and some tests.
Attachment #287152 - Attachment is obsolete: true
Attachment #287153 - Flags: review?(jst)
Attachment #287152 - Flags: superreview?(jst)
Attachment #287152 - Flags: review?(jst)
Attachment #287152 - Attachment is obsolete: false
Attachment #287152 - Flags: superreview?(jst)
Attachment #287152 - Flags: review?(jst)
Attachment #287152 - Flags: review?(cbiesinger)
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
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+
Attachment #287153 - Flags: review?(jst) → review+
Attached patch v2 (obsolete) — Splinter Review
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)
Blocks: 405693
Filed bug 405693 about the XXX in nsOfflineCacheManifestItem::OnStopRequest (shouldn't update a byte-for-byte identical cache manifest)
Blocks: 405695
Whiteboard: [has patch][has r/sr jst][needs sr biesi]
Blocks: 397416
Attached patch v3 (obsolete) — Splinter Review
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)
Attached patch tests v2 (obsolete) — Splinter Review
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+
Attached patch v4 (obsolete) — Splinter Review
(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)
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)
Status: NEW → RESOLVED
Closed: 12 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
Depends on: 491137
No longer blocks: 615923
Depends on: 615923
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.