[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 #10 from Michael Nordman <michaeln at google.com>  2012-08-28 13:03:08 PST ---
(From update of attachment 160558)
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.

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