Bug 76270 – Implement offline web applications abort API

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[huangxueqing]

Implement offline web applications API(applicationCache.abort()<http://dev.w3.org/html5/spec/Overview.html#application-cache-api>).

The abort API was different from update and swapCache, it do nothing rather than throw an InvalidStateError exception while no such applicationCache or if does not have a current application cache download process. Thus the abort() defined in DOMApplicationCache.idl without |raises(DOMException)|.

[huangxueqing]

Created attachment 122424 [details]
patch

[Early Warning System Bot]

Comment on attachment 122424 [details]
patch

Attachment 122424 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11236020

[Gustavo Noronha (kov)]

Comment on attachment 122424 [details]
patch

Attachment 122424 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11228569

[Gyuyoung Kim]

Comment on attachment 122424 [details]
patch

Attachment 122424 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11157593

[WebKit Review Bot]

Comment on attachment 122424 [details]
patch

Attachment 122424 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11115962

[huangxueqing]

Sorry, this was my first patch.
qt, gtk, efl compile error while call frame->domWindow()->console()->addMessage(...)
and chromium does not implement ApplicationCacheHost::abort().

What should i do something to fix this failures?

[Darin Adler]

Comment on attachment 122424 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122424&action=review

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:475
> +        frame->domWindow()->console()->addMessage(OtherMessageSource, LogMessageType, TipMessageLevel, "Application Cache download process was aborted.", 0, String());

The reason the addMessage call is failing to compile is the changes from r104803 and the good news is that it’s easy for you to fix. Just remove the "0, String()" arguments from the end of this line.

[huangxueqing]

(In reply to comment #7)
> (From update of attachment 122424 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122424&action=review
> 
> > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:475
> > +        frame->domWindow()->console()->addMessage(OtherMessageSource, LogMessageType, TipMessageLevel, "Application Cache download process was aborted.", 0, String());
> 
> The reason the addMessage call is failing to compile is the changes from r104803 and the good news is that it’s easy for you to fix. Just remove the "0, String()" arguments from the end of this line.

Thanks a lot. I will fix it at once. BTW, it's my fatal fault, I didn't to run  ./update-webkit before ./svn-create-patch since it a burden to update webkit from China, it normally take couple hours and connection was aborted many times :(.

[huangxueqing]

Created attachment 122437 [details]
patch to fix compile error

patch to fix compile error(GTK QT EFL)

[WebKit Review Bot]

Comment on attachment 122437 [details]
patch to fix compile error

Attachment 122437 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11152610

[Alexey Proskuryakov]

<rdar://problem/9617710>

[Alexey Proskuryakov]

Comment on attachment 122437 [details]
patch to fix compile error

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?

[Alexey Proskuryakov]

Oh, and Chromium implementation of application cache is separate. You should make sure that Chromium build doesn't break, but implementing the feature for Chromium is out of scope for this bug.

[Michael Nordman]

As for the chromium port, please make it compile with a noop abort() method and skip any new tests that rely on this method.

To make it compile, add something like the following to WebKit\chromium\src\ApplicationCacheHost.cpp

void ApplicationCacheHost::abort()
{
    // FIXME: See https://bugs.webkit.org/show_bug.cgi?id=76270
}

To skip tests, add lines to WebKit\LayoutTests\platform\chromium\test_expectations.txt that looks like...

BUGWK76270 SKIP : http/tests/appcache/<each-newly-added-tests> = FAIL

[huangxueqing]

Created attachment 122540 [details]
patch

[huangxueqing]

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

[WebKit Review Bot]

Attachment 122540 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:467:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:468:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[huangxueqing]

Created attachment 122541 [details]
patch

[huangxueqing]

(In reply to comment #14)
> As for the chromium port, please make it compile with a noop abort() method and skip any new tests that rely on this method.
> 
> To make it compile, add something like the following to WebKit\chromium\src\ApplicationCacheHost.cpp
> 
> void ApplicationCacheHost::abort()
> {
>     // FIXME: See https://bugs.webkit.org/show_bug.cgi?id=76270
> }
> 
> To skip tests, add lines to WebKit\LayoutTests\platform\chromium\test_expectations.txt that looks like...
> 
> BUGWK76270 SKIP : http/tests/appcache/<each-newly-added-tests> = FAIL

Done

[WebKit Review Bot]

Attachment 122541 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:466:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:469:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:470:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[huangxueqing]

Created attachment 122542 [details]
patch

[WebKit Review Bot]

Attachment 122542 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:467:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[huangxueqing]

Created attachment 122543 [details]
patch

Sorry, the script check-webkit-style can not run in my computer :(, I will fix it later.

[Alexey Proskuryakov]

Comment on attachment 122543 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122543&action=review

r=me

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

I'd just make this function not take any argument. That said, how we link update to a frame is so broken that it doesn't matter much.

[WebKit Review Bot]

Comment on attachment 122543 [details]
patch

Clearing flags on attachment: 122543

Committed r105085: <http://trac.webkit.org/changeset/105085>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.