Provide a way to check for ETag/Size/LastModified and update the app/cache entries if anything has changed.

Can you be more specific with the intention here? The cache and HTTP channel already work well with validation headers. What exactly it is you need to add?

The HTTP channel delegates most of the work to the packaged app service for URLs containing !// At the moment, the packaged app service does not check the headers of the package to decide if the cache entries need updating. The only workaround at the moment is trying to load a resource that does not exist in the package, and the cache miss will force the package to be downloaded and resources inserted in the cache.

I have a few options for the architecture of this: 1. Use HEAD requests to check that the package has not changed * Lookup the cache entry * In OnCacheEntryAvailable open a channel, set the method to be HEAD, and set INHIBIT_CACHING so we can download it later. * In OnStartRequest check if the response is from cache (meaning the package hasn't changed), if so, deliver OnCacheEntryAvailable to the listener. If the response isn't from the cache, we call OpenNewPackageInternal and download the package. 2. Use GET to check that the package has not changed, and cancel the channel if it hasn't * We don't look up the entry in the cache, but we open a channel for the package right away * if it's from the cache, that means it hasn't changed. We lookup the cache entry and serve it to the listener. * if it has changed, we just download it. * also, if there was an error in getting the package, I also think we should just serve it from the cache. -- I have a patch for the first option, although I think the second is better. Any thoughts?

Resolved on IRC

Comment on attachment 8632753 [details] [diff] [review] Make nsMultiMixedConv not return an error when served only a package's metadata from the cache Dealt on IRC this needs an update

Make the stream converter aware of the "application/package" mimetype.

Comment on attachment 8635639 [details] [diff] [review] Make nsMultiMixedConv not return an error when served only a package's metadata from the cache Review of attachment 8635639 [details] [diff] [review]: ----------------------------------------------------------------- Closing. Overall good. ::: netwerk/streamconv/converters/nsMultiMixedConv.cpp @@ +473,5 @@ > // in the raw stream. > mFinalListener = aListener; > > + nsAutoCString packagedAppMime(APPLICATION_PACKAGE); > + if (packagedAppMime == aFromType) { NS_LITERAL_CSTRING(APPLICATION_PACKAGE).Equals(aFromType) @@ +814,5 @@ > nsresult rv = NS_OK; > > // We should definitely have found a token at this point. Not having one > // is clearly an error, so we need to pass it to the listener. > + if (mToken.IsEmpty() && !(mPackagedApp && mIsFromCache && mFirstOnData)) { update the comment why exactly you are extending the condition, what the part you are adding exactly means. honestly I don't follow the condition at all. @@ +845,5 @@ > //(void) mFinalListener->OnStartRequest(request, ctxt); > > (void) mFinalListener->OnStopRequest(request, ctxt, aStatus); > + } else if (mIsFromCache && mFirstOnData) { > + // If the cache entry only holds metadata, then no we would not call "then no we would" few words why mFirstOnData is true here let you know there are also resources w/o content (not just because you are storing only metadata in the cache) ::: netwerk/streamconv/converters/nsMultiMixedConv.h @@ +183,5 @@ > // as it can be ascertained from the package file. > bool mPackagedApp; > + nsAutoPtr<mozilla::net::nsHttpResponseHead> mResponseHead; > + // It is necessary to know if the content is coming from the cache > + // in the case of packaged apps it would be nice to also say why

Comment on attachment 8635640 [details] [diff] [review] Add static GetPackageURI method to PackagedAppService Review of attachment 8635640 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/PackagedAppService.cpp @@ +520,5 @@ > > +/* static */ nsresult > +PackagedAppService::GetPackageURI(nsIURI *aURI, nsIURI **aPackageURI) > +{ > + nsCOMPtr<nsIURL> url = do_QueryInterface(aURI); any particular reason why not to pass nsIURL already as an arg? maybe this helps keep the code cleaner, just asking.

Comment on attachment 8635641 [details] [diff] [review] Provide way to update packaged apps Review of attachment 8635641 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/PackagedAppService.cpp @@ +398,5 @@ > } > > // If this is the last part of the package, it means the requested resources > // have not been found in the package so we return an appropriate error. > + if (NS_SUCCEEDED(aStatusCode) && lastPart && !mIsFromCache) { why has been && !mIsFromCache added to the cache? update the comment! @@ +442,5 @@ > nsCOMArray<nsICacheEntryOpenCallback>* array = mCallbacks.Get(spec); > if (array) { > + if (array->Length() == 0) { > + // This resource has already been downloaded. It's safe to just serve it > + // from the cache. "This resource download has already been done earlier, hence we don't need to wait for its population to the cache and serve it right now, directly. See also blabla method bellow" and a good catch btw! ::: netwerk/protocol/http/PackagedAppService.h @@ +150,4 @@ > }; > > + // Intercepts OnStartRequest, OnDataAvailable*, OnStopRequest method calls > + // and forwards them to the listener. what is the underlying channel? who is the target? what does this class do? what is the lifetime? ::: netwerk/test/unit/test_packaged_app_service.js @@ +195,2 @@ > function test_request_number() { > + equal(packagedAppRequestsMade, 2, "2 requests are expected. First with content, second is a 304 not modified."); hmm... how is that now it's 2? I don't see any related test changes.

> > +/* static */ nsresult > > +PackagedAppService::GetPackageURI(nsIURI *aURI, nsIURI **aPackageURI) > > +{ > > + nsCOMPtr<nsIURL> url = do_QueryInterface(aURI); > > any particular reason why not to pass nsIURL already as an arg? maybe this > helps keep the code cleaner, just asking. It seems cleaner to QI inside the method, instead of having the callers QI before calling it. Other than that there's no reason. > ::: netwerk/test/unit/test_packaged_app_service.js > @@ +195,2 @@ > > function test_request_number() { > > + equal(packagedAppRequestsMade, 2, "2 requests are expected. First with content, second is a 304 not modified."); > > hmm... how is that now it's 2? I don't see any related test changes. The test hasn't changed, but the code has. Every time a resource is requested, it opens a channel for the package. That channel may be satisfied from the cache, if its entry is still valid. But as we don't provide any Expires or cache-control headers for the package, a second request will be made, to which we respond with a 304 code.

Make the stream converter aware of the "application/package" mimetype.

Oops, forgot to qrefresh.

Comment on attachment 8636897 [details] [diff] [review] Make nsMultiMixedConv not return an error when served only a package's metadata from the cache Review of attachment 8636897 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/streamconv/converters/nsMultiMixedConv.cpp @@ +776,5 @@ > // http://www.w3.org/TR/web-packaging/#streamable-package-format > // Although it is compatible with multipart/* this format does not require > // the boundary to be included in the header, as it can be ascertained from > // the content of the file. > + if (delimiter.Find(APPLICATION_PACKAGE) != kNotFound) { would NS_LITERAL_CSTRING(APP...AGE) work here? I think there is an overload of Find() for nsACString/Substring too.. @@ +874,5 @@ > + // `mFirstOnData` is true if the package's cache entry only holds > + // metadata and no calls to OnDataAvailable are made. > + // In this case we would not call OnStopRequest for any of the parts, > + // so we need to call it here. > + (void) mFinalListener->OnStopRequest(request, ctxt, aStatus); one question: how should we behave (fix this omission) when a response from the network doesn't have any content body? ::: netwerk/streamconv/converters/nsMultiMixedConv.h @@ +184,5 @@ > bool mPackagedApp; > + nsAutoPtr<mozilla::net::nsHttpResponseHead> mResponseHead; > + // It is necessary to know if the content is coming from the cache > + // for packaged apps, in the case that only metadata is saved in the cache > + // entry, and OnDataAvailable never gets called. nit: no "," before and

> > + // so we need to call it here. > > + (void) mFinalListener->OnStopRequest(request, ctxt, aStatus); > > one question: how should we behave (fix this omission) when a response from > the network doesn't have any content body? I think not. The streamable-package-format, as well as the multipart/* format explicitly require a close-delimiter ( "--" boundary "--" ), so missing content is only valid when the response is coming from the cache. Of course, we still have the problem of hanging if we get no content. I have filed bug 1186737 to deal with the problem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/86b2cd28a91b https://hg.mozilla.org/integration/mozilla-inbound/rev/35e4460b0194 https://hg.mozilla.org/integration/mozilla-inbound/rev/396674528152

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=12042024&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/04336a8432db https://hg.mozilla.org/integration/mozilla-inbound/rev/1439e7ebd397 https://hg.mozilla.org/integration/mozilla-inbound/rev/9d92866d1ce2

https://hg.mozilla.org/integration/mozilla-inbound/rev/03fb7b080f7f https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ab9033e675 https://hg.mozilla.org/integration/mozilla-inbound/rev/84d88b744dee

https://hg.mozilla.org/mozilla-central/rev/03fb7b080f7f https://hg.mozilla.org/mozilla-central/rev/e7ab9033e675 https://hg.mozilla.org/mozilla-central/rev/84d88b744dee