Bug 37602 – AppCache Progress Events - need to include additional info per the spec

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Patrick Mueller]

per bug 24529, creating a new bug to track adding additional information to the AppCache progress events to provide information on the resources being downloaded.

The URL field in the bug points to the most recent version of a spec which defines the info.

A peer Chrome bug is open here: http://code.google.com/p/chromium/issues/detail?id=39370

Note that the spec doesn't actually mention AppCache (I believe the spec predates AppCache), but does mention XHR and Media events.  Presumably these would need to be looked at as well as just the AppCache usage.

[Michael Nordman]

Thnx for linking the bugs!

[Patrick Mueller]

Created attachment 56020 [details]
in-progress patch

First pass at changes to have the appcache "progress" event send a real ProgressEvent, with the new loadedItems and totalItems properties.  

The code is basically operational, as tested Web Inspector with a sample app I had lying around.

Issues:

- there may be a one-off issue with loadedItems
- almost sure that the appcache events will be leaked (still figuring out the RefPtr stuff)
- the totalItem count may not be quite correct
- add IBM copyrights as appropriate

Patch is against r58875, and is actually a plain old Git patch (not sure how to create an SVN friendly patch from Git)

[Michael Nordman]

Hi Patrick!

Something that you'll need to get this patch accepted is a layout test that verifies the new behavior. The appcache related layout tests can be found at.
  WebKit\LayoutTests\http\tests\appcache\...

Also I want to point out a second implementation of ApplicationCacheHost.cpp that is also in the code base.
  WebKit\chromium\src\ApplicationCacheHost.cpp

This second impl is also defined by the .h file that's in your patch. There's one edit in the .h file that will bust the chromium build.
  Vector<RefPtr<Event> > m_deferredEvents;


If you would be so kind as to retain the existing data members for us until we can catch up, that would be great... ala something like...

        DOMApplicationCache* m_domApplicationCache;
        DocumentLoader* m_documentLoader;

#if PLATFORM(CHROMIUM)
        friend class ApplicationCacheHostInternal;
        bool m_defersEvents; // Events are deferred until after document onload.
        Vector<EventID> m_deferredEvents;
        OwnPtr<ApplicationCacheHostInternal> m_internal;
#else
        friend class ApplicationCacheGroup;
        ... 
        bool m_defersEvents; // Events are deferred until after document onload.
        Vector<RefPtr<Event> > m_deferredEvents;

        // The application cache that the document loader is associated with (if any).
        RefPtr<ApplicationCache> m_applicationCache;
        ... etc ....
#endif

[Patrick Mueller]

I've posted a question to the what-wg mailing list regarding using loadedItems vs loaded:

   http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-May/026370.html

In retrospect, it's probably fine to use loaded/total instead of the new and not yet finalized -"Items" versions; it's unlikely we could provide loadedItems/totalItems for the resource counts AND use loaded/total for bytes of an individual resource, since we have no way of identifying the resource in the first place.  Perhaps that's a general slight on the Items attributes of the proposed Progress event spec.

It's also not clear when the new ProgressEvent spec would be finalized.  

So I'm thinking we use loaded/total instead of loadedItems/totalItems.  

Waiting for feedback on the ml post ...

[Patrick Mueller]

Created attachment 56509 [details]
latest in-progress patch 

I've switched to use loaded/total instead of loadedItems/totalItems in ProgressEvent.  Added a test case, but need more.  Now sending a final progress event (loaded == total).

I'm going to attempt to patch the Chromium file, if possible.  Late next week.  Let me know if trying to do this isn't really possible, or kosher; I'd prefer to not have to use interim #IFDEF hacks and just make the actual fix.

[Michael Nordman]

To get a webkit reviewer to take a look at these changes, you'll want to set the flag on the patch to r?

> I've switched to use loaded/total instead of loadedItems/totalItems in 
> ProgressEvent.  Added a test case, but need more.  Now sending a final
> progress event (loaded == total).

Right... at TPAC I thought it had been decided to move away from the dual-band progress event model for all progress events, and to stick with the simpler loaded vs total model. Where different use cases would overload loaded vs total such that it doesn't necessarily means 'bytes'.

So the switching to use them for our use is good by me.

> I'm going to attempt to patch the Chromium file, if possible.  Late next week.
> Let me know if trying to do this isn't really possible, or kosher; I'd prefer
> to not have to use interim #IFDEF hacks and just make the actual fix.

That would be nice of you. Just be aware that you won't be able to come up with total vs loaded values for chromium w/o actually making changes to chrome (outside of webkit). Maybe have the progress events in the chromium port set lengthComputable to false and total/loaded to zero for the time being.

[Michael Nordman]

915     pair<EntryMap::iterator, bool> result = m_pendingEntries.add(url, type);
916     m_total++;

I think you have to revisit how you're computing m_total. The same url may be listed multiple times in the manifest file in different sections. It only gets added to the m_pendingEntries collection and downloaded once (the return values indicates if the url was already in the collection), but m_total will have been bumped multiple times for such entries.

Also, the update algorithm can be run multiple times over the life of an ApplicationCacheGroup instance. I think you want to reset m_total and m_loaded when a new update is initiated.

[Patrick Mueller]

I've had some other things hit my radar, don't think I'll be able to continue with this for a while.  If someone gets to this before I get back to it, have at it.

Some responses to Michael:

- this code isn't ready for review :-)

- TPAC appears to be a w3 thing; are there links to "it had been decided to move away ..." discussions?  I completely concur, but it would be nice to have this info available easily.  It looks like there is some discussion here - anywhere else?

   http://www.w3.org/2009/11/02-webapps-minutes.html#item02

- re: computing "total".  I hadn't really studied this code all that closely; my assumption was that it would be possible to compute a total.  I was focusing more on getting something basically working first.  Given the mention that Chrome couldn't support total without changes to chromium (outside of webkit), I suspect it may be easier for the first actual implementation to set lengthComputable to false, set total to 0 always (except maybe the last one), and increment loaded each time the message is sent.

Other general notes:

- the existing test case is not sufficient; will need more.  As you can also note from the last patch, if we send a final progress event at the end, this will end up breaking some existing test cases, but they were easy to fix.

- really no clue if I did the PassRefPtr/etal stuff correctly.  Seemed like a best guess.  The reason why changes were needed for this was to be able to send events with data, instead of the existing bare events we have today.  To do that, the data needs to be kept in the heap; the existing code maintains a vector of eventtype ints, for queued events, which is all you need if you're not sending any data with your events ...

[Michael Nordman]

Created attachment 59278 [details]
chromium webkit api for the extra info

[Michael Nordman]

Created attachment 59279 [details]
chromium webkit api for the extra info

just fixing a typo in the ChangeLog with the second patch

[WebKit Review Bot]

Attachment 59279 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3333555

[Michael Nordman]

Created attachment 59303 [details]
chromium webkit api for the extra info

just fixing my boneheaded compile error in this rev

[Dumitru Daniliuc]

Comment on attachment 59303 [details]
chromium webkit api for the extra info

> Index: WebKit/chromium/ChangeLog
> ===================================================================
> --- WebKit/chromium/ChangeLog	(revision 61565)
> +++ WebKit/chromium/ChangeLog	(working copy)
> @@ -1,3 +1,17 @@
> [phone]  Michael Nordman  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Additional API to pass additional info needed for AppCache progress events.
> +        https://bugs.webkit.org/show_bug.cgi?id=37602
> +
> +        Also, delete an obsolete method from the worker API.
> +
> +        * public/WebApplicationCacheHostClient.h: define a new method
> +        * public/WebSharedWorker.h: remove an obsolte method
> +        * src/ApplicationCacheHostInternal.h: implement the new method
> +        (WebCore::ApplicationCacheHostInternal::notifyProgressEventListener):
> +
>  2010-06-21  Kent Tamura  <[e-mail]>
>  
>          Reviewed by Dimitri Glazkov.
> Index: WebKit/chromium/public/WebApplicationCacheHostClient.h
> ===================================================================
> --- WebKit/chromium/public/WebApplicationCacheHostClient.h	(revision 61562)
> +++ WebKit/chromium/public/WebApplicationCacheHostClient.h	(working copy)
> @@ -39,8 +39,9 @@ namespace WebKit {
>  // This interface is used by the embedder to call into webkit.
>  class WebApplicationCacheHostClient {
>  public:
> -    // Called to fire the event in the scriptable interface.
> +    // Called to fire events in the scriptable interface.
>      virtual void notifyEventListener(WebApplicationCacheHost::EventID) = 0;
> +    virtual void notifyProgressEventListener(const WebURL&, int num_total, int num_complete) = 0;
>  
>  protected:
>      // Should not be deleted by the embedder.
> Index: WebKit/chromium/public/WebSharedWorker.h
> ===================================================================
> --- WebKit/chromium/public/WebSharedWorker.h	(revision 61562)
> +++ WebKit/chromium/public/WebSharedWorker.h	(working copy)
> @@ -63,15 +63,6 @@ public:
>                                      const WebString& sourceCode,
>                                      long long scriptResourceAppCacheID) = 0;
>  
> -    // FIXME(michaeln): Remove this after the roll and adjusting to it.
> -    virtual void startWorkerContext(const WebURL& scriptURL,
> -                                    const WebString& name,
> -                                    const WebString& userAgent,
> -                                    const WebString& sourceCode)
> -    {
> -        startWorkerContext(scriptURL, name, userAgent, sourceCode, 0);
> -    }
> -
>      class ConnectListener {
>      public:
>          // Invoked once the connect event has been sent so the caller can free this object.
> Index: WebKit/chromium/src/ApplicationCacheHostInternal.h
> ===================================================================
> --- WebKit/chromium/src/ApplicationCacheHostInternal.h	(revision 61562)
> +++ WebKit/chromium/src/ApplicationCacheHostInternal.h	(working copy)
> @@ -39,6 +39,7 @@
>  #include "WebFrameImpl.h"
>  #include "WebKit.h"
>  #include "WebKitClient.h"
> +#include "WebURL.h"
>  
>  namespace WebCore {
>  
> @@ -57,6 +58,17 @@ public:
>          m_innerHost->notifyDOMApplicationCache(static_cast<ApplicationCacheHost::EventID>(eventID));
>      }
>  
> +    virtual void notifyProgressEventListener(const WebKit::WebURL&, int num_total, int num_complete) 
> +    {
> +        // FIXME: Modify webcore's progress event handling to carry the extra info and alter the
> +        // layout tests to not fail when the more recently specified 'final' event is raised.
> +        // For now, we're eating the extra info and that last event.
> +        // See https://bugs.webkit.org/show_bug.cgi?id=37602
> +        if (num_complete == num_total)
> +            return;
> +        notifyEventListener(WebKit::WebApplicationCacheHost::ProgressEvent);
> +    }
> +
>      static WebKit::WebApplicationCacheHost* toWebApplicationCacheHost(ApplicationCacheHost* innerHost)
>      {
>          if (innerHost && innerHost->m_internal.get())

WebKit/chromium/public/WebApplicationCacheHostClient.h:44
 +      virtual void notifyProgressEventListener(const WebURL&, int num_total, int num_complete) = 0;
is ApplicationCacheHostInternal is the only class that inherits from WebApplicationCacheHostClient?

WebKit/chromium/src/ApplicationCacheHostInternal.h:63
 +          // FIXME: Modify webcore's progress event handling to carry the extra info and alter the
minor: s/webcore/WebCore/

[Dumitru Daniliuc]

Heh, sorry for the spam... I need to figure out how to use the reviewing tools properly.

[Michael Nordman]

(In reply to comment #13)
 > WebKit/chromium/public/WebApplicationCacheHostClient.h:44
>  +      virtual void notifyProgressEventListener(const WebURL&, int num_total, int num_complete) = 0;
> is ApplicationCacheHostInternal is the only class that inherits from WebApplicationCacheHostClient?

Yes
 
> WebKit/chromium/src/ApplicationCacheHostInternal.h:63
>  +          // FIXME: Modify webcore's progress event handling to carry the extra info and alter the
> minor: s/webcore/WebCore/

Oh... this shouldn't be here for too long. I can upload a new patch if need be.

[Dumitru Daniliuc]

> > WebKit/chromium/src/ApplicationCacheHostInternal.h:63
> >  +          // FIXME: Modify webcore's progress event handling to carry the extra info and alter the
> > minor: s/webcore/WebCore/
> 
> Oh... this shouldn't be here for too long. I can upload a new patch if need be.

No need to, I thought you were a committer and would do it on landing. I'll cq+ the patch as soon as all bots turn green.

[WebKit Commit Bot]

Comment on attachment 59303 [details]
chromium webkit api for the extra info

Clearing flags on attachment: 59303

Committed r61856: <http://trac.webkit.org/changeset/61856>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Nordman]

Re-opening as this isn't resolved yet.

[Michael Nordman]

Assigning to myself. I've picked up where Patrick left off, will upload something for review soon.

[Michael Nordman]

Created attachment 60156 [details]
ProgressEvents

[Michael Nordman]

Comment on attachment 60156 [details]
ProgressEvents

I think this is ready for review. I started with Patrick's patch and edited to taste. The appcache layout tests pass for chromium's test_shell (win) and for webcore's dump_render_tree (mac)

[Michael Nordman]

Created attachment 60185 [details]
ProgressEvents with a style fix

New patch. Moved the bracket up a line on struct DeferredEvent {

[Michael Nordman]

Created attachment 60296 [details]
ProgressEvents with tweeks to the layout tests

ap, any interest in reviewing or should i be looking elsewhere?

[Michael Nordman]

 31     if (event.loaded != expectedLoaded++) {
 32         document.getElementById('result').innerHTML = "FAILURE: expected progressEvent.loaded to be " + expectedLoaded...
 33         done();
 34         return;
 35     }    

bah... the error message will show the wrong number as currently coded

[Michael Nordman]

Created attachment 60307 [details]
ProgressEvents with layout test error message fixed

[Michael Nordman]

49 applicationCache.addEventListener('noupdate', cached, false);

Ah, that isn't quite right. In the noupdate case, we really can't conduct this test since there won't be any progress events. I'll have test fail with a more clear error message in this case...

"FAILURE: unable to conduct test since the appcache already exists, please remove the appcache and try again"

I'd rather do that than introduce overly complicated client and server side logic to automatically delete the cache and try again... then again... maybe a LayoutTestController API to nuke the existing cache could be in order?

[Michael Nordman]

Created attachment 60787 [details]
ProgressEvents with another layout test error message fixed

New test that fails with a clear error message in the noupdate case.

[Alexey Proskuryakov]

Comment on attachment 60787 [details]
ProgressEvents with another layout test error message fixed

r=me

[Michael Nordman]

(In reply to comment #29)
> (From update of attachment 60787 [details])
> r=me

Thank you ap!

[Michael Nordman]

Comment on attachment 60787 [details]
ProgressEvents with another layout test error message fixed

This patch won't apply on top of joepecks recent changes to ApplicationCacheGroup. Removing from commit-queue.

[Michael Nordman]

Created attachment 60956 [details]
ProgressEvents merged and resolved

Same patch as last time modulo merging and resolving.

[Dumitru Daniliuc]

Comment on attachment 60956 [details]
ProgressEvents merged and resolved

rs=me, same patch that ap r+'ed.

[Eric Seidel (no email)]

Comment on attachment 60787 [details]
ProgressEvents with another layout test error message fixed

Cleared Alexey Proskuryakov's review+ from obsolete attachment 60787 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 60956 [details]
ProgressEvents merged and resolved

Clearing flags on attachment: 60956

Committed r62957: <http://trac.webkit.org/changeset/62957>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.