RESOLVED FIXED 76270
Implement offline web applications abort API
https://bugs.webkit.org/show_bug.cgi?id=76270
Summary Implement offline web applications abort API
huangxueqing
Reported 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)|.
Attachments
patch (9.39 KB, patch)
2012-01-13 06:40 PST, huangxueqing
webkit-ews: commit-queue-
patch to fix compile error (8.32 KB, patch)
2012-01-13 09:06 PST, huangxueqing
ap: review-
webkit.review.bot: commit-queue-
patch (33.68 KB, patch)
2012-01-14 04:19 PST, huangxueqing
no flags
patch (33.78 KB, patch)
2012-01-14 04:31 PST, huangxueqing
no flags
patch (33.76 KB, patch)
2012-01-14 04:43 PST, huangxueqing
no flags
patch (33.76 KB, patch)
2012-01-14 04:54 PST, huangxueqing
no flags
huangxueqing
Comment 1 2012-01-13 06:40:59 PST
Early Warning System Bot
Comment 2 2012-01-13 06:52:44 PST
Gustavo Noronha (kov)
Comment 3 2012-01-13 06:55:16 PST
Gyuyoung Kim
Comment 4 2012-01-13 06:55:25 PST
WebKit Review Bot
Comment 5 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
huangxueqing
Comment 6 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?
Darin Adler
Comment 7 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.
huangxueqing
Comment 8 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 :(.
huangxueqing
Comment 9 2012-01-13 09:06:49 PST
Created attachment 122437 [details] patch to fix compile error patch to fix compile error(GTK QT EFL)
WebKit Review Bot
Comment 10 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
Alexey Proskuryakov
Comment 11 2012-01-13 10:45:52 PST
Alexey Proskuryakov
Comment 12 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?
Alexey Proskuryakov
Comment 13 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.
Michael Nordman
Comment 14 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
huangxueqing
Comment 15 2012-01-14 04:19:03 PST
huangxueqing
Comment 16 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.
WebKit Review Bot
Comment 17 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.
huangxueqing
Comment 18 2012-01-14 04:31:42 PST
huangxueqing
Comment 19 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
WebKit Review Bot
Comment 20 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.
huangxueqing
Comment 21 2012-01-14 04:43:14 PST
WebKit Review Bot
Comment 22 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.
huangxueqing
Comment 23 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.
Alexey Proskuryakov
Comment 24 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.
WebKit Review Bot
Comment 25 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>
WebKit Review Bot
Comment 26 2012-01-16 12:51:11 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.