Bug 89230 - Add support for application cache prefer-online mode
Summary: Add support for application cache prefer-online mode
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-15 10:19 PDT by Alexey Proskuryakov
Modified: 2016-09-17 07:18 PDT (History)
12 users (show)

See Also:


Attachments
patch (24.07 KB, patch)
2012-08-11 03:57 PDT, huangxueqing
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-06 (321.62 KB, application/zip)
2012-08-11 04:40 PDT, WebKit Review Bot
no flags Details
patch (23.50 KB, patch)
2012-08-11 06:23 PDT, huangxueqing
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff
patch (43.13 KB, patch)
2012-08-25 03:10 PDT, huangxueqing
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff
patch (43.33 KB, patch)
2012-08-29 08:39 PDT, huangxueqing
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch (41.77 KB, patch)
2012-08-30 02:23 PDT, huangxueqing
no flags Details | Formatted Diff | Diff
patch (41.78 KB, patch)
2012-08-30 02:31 PDT, huangxueqing
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2012-06-15 10:19:06 PDT
There is now a way to mark a cache as preferring online resources than cached resources.

http://html5.org/tools/web-apps-tracker?from=7135&to=7136
Comment 1 Michael Nordman 2012-06-15 11:10:55 PDT
Tthe make the appcache slightly less of a douchebag feature. For more background see https://www.w3.org/Bugs/Public/show_bug.cgi?id=14702.
Comment 2 huangxueqing 2012-08-02 19:58:58 PDT
As some web developer's feedback, it's weired that we must refresh twice to make browser display the newest page, the first fetch master entry from local, then update caches in backend, and until the second refresh load the newest resource.
So, i definitly agree and interest this feature.

I suggest implementation:
* add a member in ApplicationCache named m_cacheMode default as "fast";
* if prefer-online listed in SETTINGS of manifest, set m_cacheMode as "prefer-online";
* for m_cacheMode with "prefer-online":
  * We always fetch resources from network even if resources cached in local;
  * If we fetch resources failed from network, we not only check fallback but also  explicit and master entry.

If everyone think fine, I will attach a patch to review.

thanks.
Comment 3 huangxueqing 2012-08-11 03:57:06 PDT
Created attachment 157866 [details]
patch
Comment 4 WebKit Review Bot 2012-08-11 04:40:35 PDT
Comment on attachment 157866 [details]
patch

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

New failing tests:
http/tests/appcache/prefer-online.html
Comment 5 WebKit Review Bot 2012-08-11 04:40:40 PDT
Created attachment 157867 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 6 huangxueqing 2012-08-11 06:23:02 PDT
Created attachment 157872 [details]
patch
Comment 7 Alexey Proskuryakov 2012-08-14 12:50:46 PDT
Comment on attachment 157872 [details]
patch

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

I don't think that this is implementing anything like what the spec says. The prefer-online mode changes navigation behavior, not subresource fetching. If you are actually implementing the spec, please add per-function comments in ChangeLog, explaining why each change is being made.

This patch didn't apply cleanly. It needs to be updated to apply to current tip of tree, so that EWS could check it.

> LayoutTests/http/tests/appcache/prefer-online.html:5
> +<div id="result"></duv>

Typo: duv.

> LayoutTests/http/tests/appcache/prefer-online.html:33
> +    if (load("resources/counter.php") == load("resource/counter.php")) {

The second URL is wrong here.

It's not great that a mistake like this invalidates the whole test.

> LayoutTests/http/tests/appcache/resources/prefer-online.manifest:5
> +prefer-online

Please add tests that verify how whitespace is handled. From the spec:

When the current section is the settings section, data lines must consist of zero or more U+0020 SPACE and U+0009 CHARACTER TABULATION (tab) characters, a setting, and then zero or more U+0020 SPACE and U+0009 CHARACTER TABULATION (tab) characters.

Also, please test what happens when unknown settings are encountered.

Please test empty lines in the SETTINGS section.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:446
> +    return resource ? true: false;

There is no need for the ternary operator, a pointer will be cast to a boolean implicitly.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:466
> +bool ApplicationCacheHost::scheduleLoadExcplicitResourceFromApplicationCache(ResourceLoader* loader, ApplicationCache* cache)

Typo: Excplicit.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:619
> +    executeSQLCommand("CREATE TABLE IF NOT EXISTS CacheModes (cacheMode INTERGER NOT NULL ON CONFLICT FAIL, cache INTERGER NOT NULL ON CONFLICT FAIL)");

Typo: INTERGER. How did you test this code?
Comment 8 huangxueqing 2012-08-25 03:08:53 PDT
(In reply to comment #7)
> (From update of attachment 157872 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157872&action=review
> 
> I don't think that this is implementing anything like what the spec says. The prefer-online mode changes navigation behavior, not subresource fetching. If you are actually implementing the spec, please add per-function comments in ChangeLog, explaining why each change is being made.
> 
Yes, we should only change the master entry's fetching, I have added comments in ChangeLog.

> This patch didn't apply cleanly. It needs to be updated to apply to current tip of tree, so that EWS could check it.
> 
Done

> > LayoutTests/http/tests/appcache/prefer-online.html:5
> > +<div id="result"></duv>
> 
> Typo: duv.
> 
> > LayoutTests/http/tests/appcache/prefer-online.html:33
> > +    if (load("resources/counter.php") == load("resource/counter.php")) {
> 
> The second URL is wrong here.
> 
> It's not great that a mistake like this invalidates the whole test.
> 
Sorry for typo, I have checked whole patch.

> > LayoutTests/http/tests/appcache/resources/prefer-online.manifest:5
> > +prefer-online
> 
> Please add tests that verify how whitespace is handled. From the spec:
> 
> When the current section is the settings section, data lines must consist of zero or more U+0020 SPACE and U+0009 CHARACTER TABULATION (tab) characters, a setting, and then zero or more U+0020 SPACE and U+0009 CHARACTER TABULATION (tab) characters.
> 
> Also, please test what happens when unknown settings are encountered.
> 
> Please test empty lines in the SETTINGS section.
> 
Done

> > Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:446
> > +    return resource ? true: false;
> 
> There is no need for the ternary operator, a pointer will be cast to a boolean implicitly.
> 
Done

> > Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:466
> > +bool ApplicationCacheHost::scheduleLoadExcplicitResourceFromApplicationCache(ResourceLoader* loader, ApplicationCache* cache)
> 
> Typo: Excplicit.
> 
Done

> > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:619
> > +    executeSQLCommand("CREATE TABLE IF NOT EXISTS CacheModes (cacheMode INTERGER NOT NULL ON CONFLICT FAIL, cache INTERGER NOT NULL ON CONFLICT FAIL)");
> 
> Typo: INTERGER. How did you test this code?
A case was added for this code, html file has manifest was appended to document then was removed, which destroy ApplicationCacheGroup and ApplicationCache, append html file again will load cache from database.
Comment 9 huangxueqing 2012-08-25 03:10:00 PDT
Created attachment 160558 [details]
patch
Comment 10 Michael Nordman 2012-08-28 13:03:08 PDT
Comment on attachment 160558 [details]
patch

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

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:84
> +            m_mainResourceApplicationCache = ApplicationCacheGroup::cacheForMainRequest(request, m_documentLoader);

Something may not be right here. I think a resource for the new location may be contained in a different appcache, but this logic will not pick that appcache up and is expecting to find the a resource for the new location in m_mainResourceApplicationCache. But if/when that resource is not found, line 93 will crash.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:131
> +        // in which case we just fetch resource from it.

For consistency with maybeLoadFallbackForMainResponse(), would probably be good to move this block into the if (enabled()) clause.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:375
> +    if (cache->cacheMode() == PreferOnline && resource && resource->type() & ApplicationCacheResource::Master)

Not sure these changes should be here. This method affects subresource loads which should not be affected by the prefer-online setting.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:416
> +    if (!cache) {

Why is this here? The caller passes in a non-null value.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:433
> +    return resource && resource->type() & ApplicationCacheResource::Master;

After reading the spec, looks like the test for 'Master' should not be here, but a test for 'Foreign' should be here such that 'Foreign' entries will be excluded.
Comment 11 Alexey Proskuryakov 2012-08-28 13:11:48 PDT
Comment on attachment 160558 [details]
patch

r- per Michael's comments.
Comment 12 huangxueqing 2012-08-29 08:24:26 PDT
(In reply to comment #10)
> (From update of attachment 160558 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160558&action=review
> 
> > Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:84
> > +            m_mainResourceApplicationCache = ApplicationCacheGroup::cacheForMainRequest(request, m_documentLoader);
> 
> Something may not be right here. I think a resource for the new location may be contained in a different appcache, but this logic will not pick that appcache up and is expecting to find the a resource for the new location in m_mainResourceApplicationCache. But if/when that resource is not found, line 93 will crash.
> 
This modification just avoid to retrive cache for request repeatly since this cache maybe loaded before MainResourceLoader fetch the resources from network. In addition, if a resource was included appcache as a 'Foreign', we will not pick it up. Please see ApplicationCacheStorage::cacheGroupForURL.

> > Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:131
> > +        // in which case we just fetch resource from it.
> 
> For consistency with maybeLoadFallbackForMainResponse(), would probably be good to move this block into the if (enabled()) clause.
> 
Done

> > Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:375
> > +    if (cache->cacheMode() == PreferOnline && resource && resource->type() & ApplicationCacheResource::Master)
> 
> Not sure these changes should be here. This method affects subresource loads which should not be affected by the prefer-online setting.
> 
XHR load a master entry synchronously should be affected by the prefer-online setting.

> > Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:416
> > +    if (!cache) {
> 
> Why is this here? The caller passes in a non-null value.
> 
Yes, cache was a non-null value, I just want to make consistency with getApplicationCacheFallbackResource. I use 'ASSERT(cache)' instead of it.

> > Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:433
> > +    return resource && resource->type() & ApplicationCacheResource::Master;
> 
> After reading the spec, looks like the test for 'Master' should not be here, but a test for 'Foreign' should be here such that 'Foreign' entries will be excluded.
As mentioned above, a cache included 'Foreign' resource never be picked up, therefore, I think it's fine that we use ASSERT instead of it.
Comment 13 huangxueqing 2012-08-29 08:39:14 PDT
Created attachment 161236 [details]
patch
Comment 14 Michael Nordman 2012-08-29 14:46:58 PDT
There's another aspect of prefer-online that I'm not sure is covered in this patch. How are the contents of the appcache utilized for subresource loads in this case?

> > Something may not be right here. I think a resource for the new location may be contained in a different appcache, but this logic will not pick that appcache up and is expecting to find the a resource for the new location in m_mainResourceApplicationCache. But if/when that resource is not found, line 93 will crash.
> > 
> This modification just avoid to retrive cache for request repeatly since this cache maybe loaded before MainResourceLoader fetch the resources from network.

Maybe I'm confused... but... when this is invoked a 2nd time via the callsite in maybeLoadMainResourceForRedirect(), request->url() will have a different value corresponding to the location of the redirect (right?). Theres is no guarantee that the cache identified on the first invocation (with the original url) will contain a resource for this new url, and there may actually be another cache that contains a resource for that new url.

> 
> > Not sure these changes should be here. This method affects subresource loads which should not be affected by the prefer-online setting.
> > 
> XHR load a master entry synchronously should be affected by the prefer-online setting.

XHR always performs subresource loads, "main" vs "sub" is not function of the resource being loaded, its a function of how its being loaded. 

> > After reading the spec, looks like the test for 'Master' should not be here, but a test for 'Foreign' should be here such that 'Foreign' entries will be excluded.
> As mentioned above, a cache included 'Foreign' resource never be picked up, therefore, I think it's fine that we use ASSERT instead of it.

Great, maybe we can ASSERT(!Foreign) here then. But still, I don't see where the test for 'Master' is correct. An explicitly listed resource that's not a 'Master' can also be navigated to and an appcached resource should be utilized in that case too.
Comment 15 WebKit Review Bot 2012-08-29 16:29:57 PDT
Comment on attachment 161236 [details]
patch

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

New failing tests:
CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread
Comment 16 huangxueqing 2012-08-30 02:20:52 PDT
(In reply to comment #14)
> There's another aspect of prefer-online that I'm not sure is covered in this patch. How are the contents of the appcache utilized for subresource loads in this case?
> 
I'm sorry i did not understand you means totally. Do you means this patch how to affect subresource fetching? If so, as spec said, "prefer-online" only change navigation, this patch did not change any behavior of subresource.

> > > Something may not be right here. I think a resource for the new location may be contained in a different appcache, but this logic will not pick that appcache up and is expecting to find the a resource for the new location in m_mainResourceApplicationCache. But if/when that resource is not found, line 93 will crash.
> > > 
> > This modification just avoid to retrive cache for request repeatly since this cache maybe loaded before MainResourceLoader fetch the resources from network.
> 
> Maybe I'm confused... but... when this is invoked a 2nd time via the callsite in maybeLoadMainResourceForRedirect(), request->url() will have a different value corresponding to the location of the redirect (right?). Theres is no guarantee that the cache identified on the first invocation (with the original url) will contain a resource for this new url, and there may actually be another cache that contains a resource for that new url.
> 
Yes, you're right, thanks for remind, we should reset m_mainResourceApplicationCache before retrieve appcache for redirected request, it try to load from appcache always fail since appcache's mode was 'prefer-online'. I had changed it.

> > 
> > > Not sure these changes should be here. This method affects subresource loads which should not be affected by the prefer-online setting.
> > > 
> > XHR load a master entry synchronously should be affected by the prefer-online setting.
> 
> XHR always performs subresource loads, "main" vs "sub" is not function of the resource being loaded, its a function of how its being loaded. 
> 
Oh~, do you means even if XHR load a "**.html" should treat as subresource? It seems reasonable, alright, i removed this piece of code.

> > > After reading the spec, looks like the test for 'Master' should not be here, but a test for 'Foreign' should be here such that 'Foreign' entries will be excluded.
> > As mentioned above, a cache included 'Foreign' resource never be picked up, therefore, I think it's fine that we use ASSERT instead of it.
> 
> Great, maybe we can ASSERT(!Foreign) here then. But still, I don't see where the test for 'Master' is correct. An explicitly listed resource that's not a 'Master' can also be navigated to and an appcached resource should be utilized in that case too.
Consider 'a' html file was listed in 'b' manifest file, 'a' has included another manifest 'c', that implicit 'a' was marked as 'Foreign' in 'b', appcache identified by 'b' nerver be picked up;
If 'a' has not include any manifest file, this seem not specified in spec, in current implementation, 'a' will be picked up since 'a' was 'Explicit' in b. Therefore, I did not prefer to add a test for this.
Comment 17 huangxueqing 2012-08-30 02:22:45 PDT
(In reply to comment #15)
> (From update of attachment 161236 [details])
> Attachment 161236 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13694438
> 
> New failing tests:
> CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread

Oops~ I don't why this test was failed, try to re-attach patch.
Comment 18 huangxueqing 2012-08-30 02:23:33 PDT
Created attachment 161430 [details]
patch
Comment 19 WebKit Review Bot 2012-08-30 02:27:31 PDT
Attachment 161430 [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:83:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:84:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 huangxueqing 2012-08-30 02:31:09 PDT
Created attachment 161432 [details]
patch
Comment 21 huangxueqing 2012-09-04 08:06:51 PDT
Any comments?thanks
Comment 22 Anders Carlsson 2014-02-05 11:12:16 PST
Comment on attachment 161432 [details]
patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
Comment 23 Michael Catanzaro 2016-09-17 07:18:23 PDT
Comment on attachment 161432 [details]
patch

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.