[Webkit-unassigned] [Bug 89230] Add support for application cache prefer-online mode

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=89230





--- Comment #12 from huangxueqing <huangxueqing at baidu.com>  2012-08-29 08:24:26 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.