Last Comment Bug 767096 - Optimize nsICachingChannel interface for offline cache writing
: Optimize nsICachingChannel interface for offline cache writing
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
: 766402 (view as bug list)
Depends on:
Blocks: 739868
  Show dependency treegraph
 
Reported: 2012-06-21 12:35 PDT by Honza Bambas (:mayhemer)
Modified: 2012-07-12 10:56 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (36.17 KB, patch)
2012-06-21 12:35 PDT, Honza Bambas (:mayhemer)
tlee: review+
honzab.moz: checkin+
Details | Diff | Splinter Review

Description Honza Bambas (:mayhemer) 2012-06-21 12:35:30 PDT
Created attachment 635416 [details] [diff] [review]
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 1 Honza Bambas (:mayhemer) 2012-06-21 12:36:35 PDT
*** Bug 766402 has been marked as a duplicate of this bug. ***
Comment 2 Honza Bambas (:mayhemer) 2012-06-21 13:22:33 PDT
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] 2012-06-22 03:52:28 PDT
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) 2012-06-29 09:19:29 PDT
(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) 2012-07-02 06:09:43 PDT
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] 2012-07-05 04:34:41 PDT
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 8 Honza Bambas (:mayhemer) 2012-07-10 14:55:29 PDT
(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] 2012-07-10 16:45:32 PDT
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 11 Ed Morley [:emorley] 2012-07-12 09:38:58 PDT
https://hg.mozilla.org/mozilla-central/rev/21a7618affa8
Comment 12 Honza Bambas (:mayhemer) 2012-07-12 10:56:38 PDT
The followup is now part of the checkin.  Forgot to remove LEAVE OPEN.  Resolving as fixed.

Note You need to log in before you can comment on or make changes to this bug.