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

[bugzilla-daemon at webkit.org]

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


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #122437|review?                     |review-
               Flag|                            |




--- Comment #12 from Alexey Proskuryakov <ap at webkit.org>  2012-01-13 11:19:05 PST ---
(From update of attachment 122437)
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.

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

> 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.

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

> Source/WebCore/loader/appcache/ApplicationCacheGroup.h:80
> +    void abort(Frame*);

What's the expected behavior if frame argument is different from m_frame?

> 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?

> 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?

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