[webkit-reviews] review denied: [Bug 76270] implement offline web applications abort API : [Attachment 122437] patch to fix compile error

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied huangxueqing
<huangxueqing at baidu.com>'s request for review:
Bug 76270: implement offline web applications abort API
https://bugs.webkit.org/show_bug.cgi?id=76270

Attachment 122437: patch to fix compile error
https://bugs.webkit.org/attachment.cgi?id=122437&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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?