Add support for application cache prefer-online mode

[Alexey Proskuryakov]

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

[Michael Nordman]

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.

[huangxueqing]

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.

[huangxueqing]

Created attachment 157866 [details]
patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[huangxueqing]

Created attachment 157872 [details]
patch

[Alexey Proskuryakov]

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?

[huangxueqing]

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

[huangxueqing]

Created attachment 160558 [details]
patch

[Michael Nordman]

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.

[Alexey Proskuryakov]

Comment on attachment 160558 [details]
patch

r- per Michael's comments.

[huangxueqing]

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

[huangxueqing]

Created attachment 161236 [details]
patch

[Michael Nordman]

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.

[WebKit Review Bot]

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

[huangxueqing]

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

[huangxueqing]

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

[huangxueqing]

Created attachment 161430 [details]
patch

[WebKit Review Bot]

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.

[huangxueqing]

Created attachment 161432 [details]
patch

[huangxueqing]

Any comments？thanks

[Anders Carlsson]

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.

[Michael Catanzaro]

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.

[Anne van Kesteren]

AppCache is now disabled and will be removed in bug 219391.