767096 - Optimize nsICachingChannel interface for offline cache writing

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

Attachment: v1

- removes cacheForOfflineUse, cacheForOfflineUse, profileDirectory from nsICachingChannel
- adds applicationCacheForWrite to nsIApplicationCacheChannel
- logic in nsHttpChannel and nsOfflineCacheUpdate updated accordingly
- fixes an existing bug when a partial update didn't update mClientID when didn't find it's appcache by the originally given clientID (however, I believe it has never happened)
- renames nsIApplicationCache.cacheDirectory to profileDirectory (actually fixes bug 766402)

Not sure who should review this, assign to Jason.  Jason, feel free to delegate to anyone you think would more suitable.

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 635416 [details] [diff] [review]
v1

One note: I'm changing HttpCacheQuery::mCacheForOfflineUse to a ref to mApplicationCacheForWrite.  But that is not needed, since HttpCacheQuery is only using it for information whether the channel writes to offline cache.  In the next version I will revert this change back.

BTW, here is a try run for v1 patch: https://tbpl.mozilla.org/?tree=Try&rev=6b81cb9ea850

Comment 3

[Thinker Li [:sinker]]

Since an nsIApplicationCache was assigned to the nsHttpChannel, is there any reason to invoke HttpCacheQuery before calling OpenOfflineCacheEntryForWriting()?  Even without assigning a nsIApplicationCache, it is still weird to call HttpCacheQuery before calling Open*Writing().

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Thinker Li [:sinker] from comment #3)
> Since an nsIApplicationCache was assigned to the nsHttpChannel, is there any
> reason to invoke HttpCacheQuery before calling
> OpenOfflineCacheEntryForWriting()?  Even without assigning a
> nsIApplicationCache, it is still weird to call HttpCacheQuery before calling
> Open*Writing().

Not sure I follow.  HttpCacheQuery opens entries for reading.  We are (may be) reading while writing to the offline cache, since the old cache is simply used as an HTTP cache for the load (that is by the spec and common sense).

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 635416 [details] [diff] [review]
v1

Reassigning to Thinker.  We need new people be able to review code around here.

Comment 6

[Thinker Li [:sinker]]

Comment on attachment 635416 [details] [diff] [review]
v1

Review of attachment 635416 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1224,5 @@
>  NS_IMETHODIMP
> +HttpChannelChild::GetApplicationCacheForWrite(nsIApplicationCache **aApplicationCache)
> +{
> +  *aApplicationCache = nsnull;
> +  return NS_OK;

Why don't you return NS_ERROR_NOT_IMPLEMENTED or some error code since this is supposed to be not used in child?

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ -1291,5 @@
>  
>      LOG(("nsOfflineCacheUpdate::InitPartial [%p]", this));
>  
>      mPartialUpdate = true;
> -    mClientID = clientID;

mClient is no more used, but it is not removed from the declaration of the nsOfflineCacheUpdate.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 635416 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/991404facac2

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Thinker Li [:sinker] from comment #6)
> Comment on attachment 635416 [details] [diff] [review]
> v1
> 
> Review of attachment 635416 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +1224,5 @@
> >  NS_IMETHODIMP
> > +HttpChannelChild::GetApplicationCacheForWrite(nsIApplicationCache **aApplicationCache)
> > +{
> > +  *aApplicationCache = nsnull;
> > +  return NS_OK;
> 
> Why don't you return NS_ERROR_NOT_IMPLEMENTED or some error code since this
> is supposed to be not used in child?

I think this is better.  Getters should not throw unless really necessary IMO.  This implementation is OK for the purpose.

> 
> ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
> @@ -1291,5 @@
> >  
> >      LOG(("nsOfflineCacheUpdate::InitPartial [%p]", this));
> >  
> >      mPartialUpdate = true;
> > -    mClientID = clientID;
> 
> mClient is no more used, but it is not removed from the declaration of the
> nsOfflineCacheUpdate.

I will land this followups as a separate checkin.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

http://mozillamemes.tumblr.com/post/21637966463/yes-i-have-a-condition-that-forces-me-to-insert

One of the patches was causing xpcshell failures. Since I didn't see Try results in any of the bugs, I had to backout the entire push.
https://hg.mozilla.org/integration/mozilla-inbound/rev/981ac887f6e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/925562fd7d68

An example:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13404043&tree=Mozilla-Inbound

TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js | running test ...
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>

TEST-INFO | (xpcshell/head.js) | test 1 pending

TEST-INFO | (xpcshell/head.js) | test 2 pending

TEST-INFO | (xpcshell/head.js) | test 2 finished

TEST-INFO | (xpcshell/head.js) | running event loop

TEST-INFO | (xpcshell/head.js) | test 2 pending
TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js | Starting test_normal

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/head.js | [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js :: make_channel_for_offline_use :: line 21"  data: no] - See following stack:
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 451
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: _run_next_test :: line 899
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: <TOP_LEVEL> :: line 418

TEST-INFO | (xpcshell/head.js) | exiting test

TEST-INFO | (xpcshell/head.js) | test 2 finished
<<<<<<<

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 635416 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/21a7618affa8

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/21a7618affa8

Comment 12

[Honza Bambas (:mayhemer)]

The followup is now part of the checkin.  Forgot to remove LEAVE OPEN.  Resolving as fixed.