Bug 76270

Summary: Implement offline web applications abort API
Product: WebKit Reporter: huangxueqing <huangxueqing>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, ddkilzer, dglazkov, gns, japhet, michaeln, ojan, webkit.review.bot, xan.lopez
Priority: P2 Keywords: HTML5, InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=87633
Attachments:
Description Flags
patch
webkit-ews: commit-queue-
patch to fix compile error
ap: review-, webkit.review.bot: commit-queue-
patch
none
patch
none
patch
none
patch none

Description huangxueqing 2012-01-13 06:14:01 PST
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)|.
Comment 1 huangxueqing 2012-01-13 06:40:59 PST
Created attachment 122424 [details]
patch
Comment 2 Early Warning System Bot 2012-01-13 06:52:44 PST
Comment on attachment 122424 [details]
patch

Attachment 122424 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11236020
Comment 3 Gustavo Noronha (kov) 2012-01-13 06:55:16 PST
Comment on attachment 122424 [details]
patch

Attachment 122424 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11228569
Comment 4 Gyuyoung Kim 2012-01-13 06:55:25 PST
Comment on attachment 122424 [details]
patch

Attachment 122424 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11157593
Comment 5 WebKit Review Bot 2012-01-13 07:03:03 PST
Comment on attachment 122424 [details]
patch

Attachment 122424 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11115962
Comment 6 huangxueqing 2012-01-13 07:11:40 PST
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?
Comment 7 Darin Adler 2012-01-13 08:38:04 PST
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.
Comment 8 huangxueqing 2012-01-13 09:02:41 PST
(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 :(.
Comment 9 huangxueqing 2012-01-13 09:06:49 PST
Created attachment 122437 [details]
patch to fix compile error

patch to fix compile error(GTK QT EFL)
Comment 10 WebKit Review Bot 2012-01-13 09:51:17 PST
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
Comment 11 Alexey Proskuryakov 2012-01-13 10:45:52 PST
<rdar://problem/9617710>
Comment 12 Alexey Proskuryakov 2012-01-13 11:19:05 PST
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?
Comment 13 Alexey Proskuryakov 2012-01-13 11:20:44 PST
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.
Comment 14 Michael Nordman 2012-01-13 13:01:23 PST
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
Comment 15 huangxueqing 2012-01-14 04:19:03 PST
Created attachment 122540 [details]
patch
Comment 16 huangxueqing 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.
Comment 17 WebKit Review Bot 2012-01-14 04:20:56 PST
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.
Comment 18 huangxueqing 2012-01-14 04:31:42 PST
Created attachment 122541 [details]
patch
Comment 19 huangxueqing 2012-01-14 04:32:17 PST
(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
Comment 20 WebKit Review Bot 2012-01-14 04:34:25 PST
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.
Comment 21 huangxueqing 2012-01-14 04:43:14 PST
Created attachment 122542 [details]
patch
Comment 22 WebKit Review Bot 2012-01-14 04:46:12 PST
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.
Comment 23 huangxueqing 2012-01-14 04:54:07 PST
Created attachment 122543 [details]
patch

Sorry, the script check-webkit-style can not run in my computer :(, I will fix it later.
Comment 24 Alexey Proskuryakov 2012-01-16 11:04:46 PST
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.
Comment 25 WebKit Review Bot 2012-01-16 12:51:03 PST
Comment on attachment 122543 [details]
patch

Clearing flags on attachment: 122543

Committed r105085: <http://trac.webkit.org/changeset/105085>
Comment 26 WebKit Review Bot 2012-01-16 12:51:11 PST
All reviewed patches have been landed.  Closing bug.