WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
patch to fix compile error
(8.32 KB, patch)
2012-01-13 09:06 PST
,
huangxueqing
ap
: review-
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
patch
(33.68 KB, patch)
2012-01-14 04:19 PST
,
huangxueqing
no flags
Details
Formatted Diff
Diff
patch
(33.78 KB, patch)
2012-01-14 04:31 PST
,
huangxueqing
no flags
Details
Formatted Diff
Diff
patch
(33.76 KB, patch)
2012-01-14 04:43 PST
,
huangxueqing
no flags
Details
Formatted Diff
Diff
patch
(33.76 KB, patch)
2012-01-14 04:54 PST
,
huangxueqing
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
huangxueqing
Comment 1
2012-01-13 06:40:59 PST
Created
attachment 122424
[details]
patch
Early Warning System Bot
Comment 2
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
Gustavo Noronha (kov)
Comment 3
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
Gyuyoung Kim
Comment 4
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
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
<
rdar://problem/9617710
>
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
Created
attachment 122540
[details]
patch
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
Created
attachment 122541
[details]
patch
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
Created
attachment 122542
[details]
patch
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.
Top of Page
Format For Printing
XML
Clone This Bug