Closed Bug 744710 Opened 8 years ago Closed 7 years ago

Enable getting updates on how many bytes of an appcache update have been downloaded

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15
blocking-kilimanjaro +

People

(Reporter: sicking, Assigned: mayhemer)

Details

Attachments

(1 file, 1 obsolete file)

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
Over to Honza in hopes that he can help out here.
Assignee: nobody → honzab.moz
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.
This blocks our webapps story, so blocking kilimanjaro.
blocking-kilimanjaro: --- → +
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.
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.
Attached patch v1 (obsolete) — Splinter Review
Attachment #621038 - Flags: review?(dcamp)
Status: NEW → ASSIGNED
Attachment #621038 - Flags: review?(dcamp) → review?(michal.novotny)
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?
(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 ;)
(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.
Attached patch v1.1Splinter Review
Attachment #621038 - Attachment is obsolete: true
Attachment #621038 - Flags: review?(michal.novotny)
Attachment #623726 - Flags: review?(michal.novotny)
Attachment #623726 - Flags: review?(michal.novotny) → review+
Honza, is this ready to land? If so, can you land this, or let me know to find someone else to do so? Thanks!
https://hg.mozilla.org/mozilla-central/rev/10b04214089d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.