[Webkit-unassigned] [Bug 76270] Implement offline web applications abort API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76270





--- Comment #16 from huangxueqing <huangxueqing at baidu.com>  2012-01-14 04:19:57 PST ---
(In reply to comment #12)
> (From update of attachment 122437 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122437&action=review
> 
> We need more tests for this feature. Anything that didn't work at first in your implementation, or anything that you suspect a future careless refactoring could potentially break should be tested.
> 
> Don't be shy to add multiple test files if needed - this often works better than cramming multiple tests into one HTML file.
> 
Done

> > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:465
> > +    if (m_updateStatus == Checking || m_updateStatus == Downloading) {
> 
> In WebKit code, we prefer early return to nesting code. So, just write 
> 
> if (m_updateStatus == Idle)
>     return;
> ASSERT(m_updateStatus == Checking || (m_updateStatus == Downloading && m_cacheBeingUpdated));
> 
Done

> > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:470
> > +        // It hasn't necessary abort download process if 
> > +        // m_completionType was Failure or Completed or NoUpdate 
> > +        // since the process was completed except reset state.
> > +        if (m_completionType != None)
> > +            return;
> 
> When does this code work? Is it for calling abort() from within an error or cached event handler? But I would expect that to be caught by m_updateStatus check above.
> 
> Please verify that this code is really needed, and add a regression test that would have failed if not for it.
> 
This condition used to this scenario: abort() was called when ApplicationCacheGroup::deliverDelayedMainResources was running(now m_updateStatus was Downloading), and if we call cacheUpdateFailed() in abort(), deliverDelayedMainResources() maybe run twice and will fire ERROR_EVENT twice. I could not find a proper event to test this scene, but i think should prevent it.

> > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:472
> > +        stopLoading();
> 
> The spec just says that we should run cache failure steps, which translates to calling cacheUpdateFailed(). Is there a reason why that doesn't work?
> 
> I'm very concerned about adding another copy of cleanup code - historically, it's been extremely fragile and difficult to maintain.
> 
> We need a regression test showing that a previous version of appcache is still fully alive and functioning after an abort().
> 
As mentioned above, i want to prevent error or cached event was fired twice, but m_completeType is enough, cacheUpdateFailed() insteaded of this block code. 

> > Source/WebCore/loader/appcache/ApplicationCacheGroup.h:80
> > +    void abort(Frame*);
> 
> What's the expected behavior if frame argument is different from m_frame?
> 
Actually, the html5 not specify this behavior, i thought maybe we can call m_frame->domWindow()->console()->addMessage(OtherMessageSource, LogMessageType, TipMessageLevel, "Application Cache download process was aborted by other frame."); But i'm not sure, i will be fine if you think this should be added.

> > Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:468
> > +    if ((status() == UNCACHED)
> > +        || (status() == OBSOLETE)
> > +        || (status() == UPDATEREADY)
> > +        || (status() == IDLE))
> > +        return;
> 
> We already have a similar check in ApplicationCacheGroup. Why is it repeated here?
> 
Done

> > Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:471
> > +    ApplicationCache* cache = applicationCache();
> > +    cache->group()->abort(m_documentLoader->frame());
> 
> A comment in ApplicationCacheHost.h says that this is "The application cache that the document loader is associated with (if any)". But a document loader isn't associated with a cache until it's fully downloaded. So how can we stop an initial download with this code? I'm confused.
> 
> Shouldn't we be using m_candidateApplicationCacheGroup?

This had some confusion, i reviewed these code carefully today. m_candidateApplicationCacheGroup was assigned a valid cacheGroup in ApplicationCacheGroup::selectCache. it was cleaned while manifest was parsed completely and m_cacheBeingUpdated was associated with host. And now ApplicationCacheHost::m_applicationCache was valid. Thus, we should using m_candidateApplicationCacheGroup from Checking to Downloading while m_applicationCache from Downloading to the last.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.