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

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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)]

Over to Honza in hopes that he can help out here.

Comment 2

[Honza Bambas (:mayhemer)]

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)]

This blocks our webapps story, so blocking kilimanjaro.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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)]

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)]

Attachment: v1

Comment 7

[Michal Novotny [:michal]]

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)]

(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]]

(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)]

Attachment: v1.1

Comment 11

[Johnny Stenback (:jst)]

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 12

[Honza Bambas (:mayhemer)]

Comment on attachment 623726 [details] [diff] [review]
v1.1

https://hg.mozilla.org/integration/mozilla-inbound/rev/10b04214089d

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/10b04214089d