Last Comment Bug 744710 - Enable getting updates on how many bytes of an appcache update have been downloaded
: Enable getting updates on how many bytes of an appcache update have been down...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-12 01:41 PDT by Jonas Sicking (:sicking)
Modified: 2012-05-24 17:09 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
v1 (13.46 KB, patch)
2012-05-04 06:51 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Review
v1.1 (14.03 KB, patch)
2012-05-14 11:09 PDT, Honza Bambas (:mayhemer)
michal.novotny: review+
honzab.moz: checkin+
Details | Diff | Review

Description Jonas Sicking (:sicking) 2012-04-12 01:41:41 PDT
Currently you can only get information about how many items of an appcache has been downloaded. You can't get information about how many bytes have been downloaded. This means that if you want to display a progress bar, you have to guess that all resources are of the same size, which can result in a very unreliable progress bar.

Would be great to expose this information through for example a new function on nsIOfflineCacheUpdateObserver
Comment 1 Johnny Stenback (:jst, jst@mozilla.com) 2012-05-02 17:26:42 PDT
Over to Honza in hopes that he can help out here.
Comment 2 Honza Bambas (:mayhemer) 2012-05-03 15:38:44 PDT
From point of view of web content, we have to have a new event (mozByteProgress maybe) that will fire with some reasonable granularity and give the overall number of bytes so far downloaded.  Depends on if this is needed, but maybe now according the description.

From point of view of C++ nsIOfflineCacheUpdateObserver can have a new event implemented for which updateStateChanges can be fired (like STATE_ITEMPROGRESS).  nsIOfflineCacheUpdate then can expose the overall number of bytes downloaded through a new r/o attribute.

Items already expose the number of downloaded bytes via nsIDOMLoadStatus (loadedSize).  But items are not exposed anywhere.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2012-05-03 15:58:13 PDT
This blocks our webapps story, so blocking kilimanjaro.
Comment 4 Jonas Sicking (:sicking) 2012-05-03 16:15:38 PDT
I wouldn't worry about exposing this to web content yet. It's enough to expose it to our internal APIs and then we'll expose it through to webapp installation api.

I certainly think that we should expose it to web content eventually too, but that's lower priority and I think we need to come up with a comprehensive plan for how we want to fix the appcache for web content anyway.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2012-05-03 16:20:44 PDT
Agreed, especially given the tight schedule. The less we do here the more time we have for the other work, so let's delay exposing this to web content for now.
Comment 6 Honza Bambas (:mayhemer) 2012-05-04 06:51:56 PDT
Created attachment 621038 [details] [diff] [review]
v1
Comment 7 Michal Novotny (:michal) 2012-05-10 07:22:29 PDT
Comment on attachment 621038 [details] [diff] [review]
v1

>    const unsigned long STATE_ITEMCOMPLETED = 7;
> +  const unsigned long STATE_ITEMPROGRESS = 11;
>    const unsigned long STATE_FINISHED = 10;

Why did you choose 11?


> +    if (mUpdate)
> +        mUpdate->OnByteProgress(bytesRead);

AFAICS this can never be null. And if it could then we would miss some progress update so nsOfflineCacheUpdate::mByteProgress would not be correct.


> +OfflineCacheUpdateChild::GetByteProgress(PRUint64 * _result)
> +{
> +    NS_ENSURE_ARG(_result);
> +    // TODO Make this available on e10s content processes too
> +    *_result = 0;
> +}

Shouldn't you return mByteProgress here?
Comment 8 Honza Bambas (:mayhemer) 2012-05-10 07:27:48 PDT
(In reply to Michal Novotny (:michal) from comment #7)
> Comment on attachment 621038 [details] [diff] [review]
> v1
> 
> >    const unsigned long STATE_ITEMCOMPLETED = 7;
> > +  const unsigned long STATE_ITEMPROGRESS = 11;
> >    const unsigned long STATE_FINISHED = 10;
> 
> Why did you choose 11?

Just to be save we don't overlap with anything older.

> 
> 
> > +    if (mUpdate)
> > +        mUpdate->OnByteProgress(bytesRead);
> 
> AFAICS this can never be null. And if it could then we would miss some
> progress update so nsOfflineCacheUpdate::mByteProgress would not be correct.

Yes, there is no need for a non-null check.

> 
> 
> > +OfflineCacheUpdateChild::GetByteProgress(PRUint64 * _result)
> > +{
> > +    NS_ENSURE_ARG(_result);
> > +    // TODO Make this available on e10s content processes too
> > +    *_result = 0;
> > +}
> 
> Shouldn't you return mByteProgress here?

Ha.  Yes.  I should.

And also return NS_OK; is missing ;)
Comment 9 Michal Novotny (:michal) 2012-05-10 07:34:21 PDT
(In reply to Honza Bambas (:mayhemer) from comment #8)
> (In reply to Michal Novotny (:michal) from comment #7)
> > Comment on attachment 621038 [details] [diff] [review]
> > v1
> > 
> > >    const unsigned long STATE_ITEMCOMPLETED = 7;
> > > +  const unsigned long STATE_ITEMPROGRESS = 11;
> > >    const unsigned long STATE_FINISHED = 10;
> > 
> > Why did you choose 11?

It seems to me that 8 and 9 were never used (http://hg.mozilla.org/mozilla-central/rev/c73c0da830fe), but I'm fine with 11.
Comment 10 Honza Bambas (:mayhemer) 2012-05-14 11:09:09 PDT
Created attachment 623726 [details] [diff] [review]
v1.1
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2012-05-22 09:31:45 PDT
Honza, is this ready to land? If so, can you land this, or let me know to find someone else to do so? Thanks!
Comment 13 Ed Morley [:emorley] 2012-05-23 05:20:54 PDT
https://hg.mozilla.org/mozilla-central/rev/10b04214089d

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