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.
Thnx for linking the bugs!
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)
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
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 ...
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.
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.
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.
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 ...
Created attachment 59278 [details] chromium webkit api for the extra info
Created attachment 59279 [details] chromium webkit api for the extra info just fixing a typo in the ChangeLog with the second patch
Attachment 59279 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3333555
Created attachment 59303 [details] chromium webkit api for the extra info just fixing my boneheaded compile error in this rev
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 @@ > +2010-06-21 Michael Nordman <michaeln@google.com> > + > + 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 <tkent@chromium.org> > > 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/
Heh, sorry for the spam... I need to figure out how to use the reviewing tools properly.
(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.
> > 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.
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>
All reviewed patches have been landed. Closing bug.
Re-opening as this isn't resolved yet.
Assigning to myself. I've picked up where Patrick left off, will upload something for review soon.
Created attachment 60156 [details] ProgressEvents
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)
Created attachment 60185 [details] ProgressEvents with a style fix New patch. Moved the bracket up a line on struct DeferredEvent {
Created attachment 60296 [details] ProgressEvents with tweeks to the layout tests ap, any interest in reviewing or should i be looking elsewhere?
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
Created attachment 60307 [details] ProgressEvents with layout test error message fixed
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?
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.
Comment on attachment 60787 [details] ProgressEvents with another layout test error message fixed r=me
(In reply to comment #29) > (From update of attachment 60787 [details]) > r=me Thank you ap!
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.
Created attachment 60956 [details] ProgressEvents merged and resolved Same patch as last time modulo merging and resolving.
Comment on attachment 60956 [details] ProgressEvents merged and resolved rs=me, same patch that ap r+'ed.
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.
Comment on attachment 60956 [details] ProgressEvents merged and resolved Clearing flags on attachment: 60956 Committed r62957: <http://trac.webkit.org/changeset/62957>