[Webkit-unassigned] [Bug 64177] ApplicationCache update should not immediately fail when reaching per-origin quota

[bugzilla-daemon at webkit.org]

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


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #100138|review?                     |review-
               Flag|                            |




--- Comment #6 from Alexey Proskuryakov <ap at webkit.org>  2011-07-08 11:41:20 PST ---
(From update of attachment 100138)
View in context: https://bugs.webkit.org/attachment.cgi?id=100138&action=review

Looks good for the most part.

Many of my comments are for discussion, although I expect at least some of them to lead to code changes. Not accounting for other caches with the same origin seems like a clear mistake.

> LayoutTests/ChangeLog:4
> +        ApplicationCache update should not immediately fail when reaching per-origin quota
> +        https://bugs.webkit.org/show_bug.cgi?id=64177

ChangeLog needs an overview of the new behavior. There are at least two ways to fix the bug, the other being that we wait for user decision right after exceeding the quota (in that case, the user could get multiple prompts).

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:812
> +void ApplicationCacheGroup::didReachOriginQuota(PassRefPtr<Frame> frame, int64_t spaceNeeded)
>  {
>      // Inform the client the origin quota has been reached,
>      // they may decide to increase the quota.
> -    frame->page()->chrome()->client()->reachedApplicationCacheOriginQuota(m_origin.get());
> +    frame->page()->chrome()->client()->reachedApplicationCacheOriginQuota(m_origin.get(), spaceNeeded);

This seems wrong. The quota is per origin, and there may be other caches using part of it. You need to add up other caches sizes when asking the client.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:909
> +        // If we encountered the origin quota while downloading we can request a quota
> +        // increase now, before we attempt to store the cache.
> +        if (m_originQuotaReachedWhileDownloading)

Why do we need to pre-compute m_originQuotaReachedWhileDownloading? It seems that with the proposed logic, we can just compare the final size to quota here.

You also have console logging when this happens, but it seems that it would be more helpful to log the full size at the end, not just tell the developer which resource brought us over the limit.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.h:122
> -    void didReachOriginQuota(PassRefPtr<Frame> frame);
> +    void didReachOriginQuota(PassRefPtr<Frame> frame, int64_t spaceNeeded);

I think that the pre-exising use of PassRefPtr is incorrect, and it would be good to fix it now. We had a long discussion about that in webkit-dev recently, and it seemed that Darin and myself had the last word :)

> Source/WebCore/loader/appcache/ApplicationCacheGroup.h:205
> +    bool m_originQuotaReachedWhileDownloading;

"DuringCurrentUpdate" perhaps? Or maybe the variable is not needed at all.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.h:206
> +    bool m_originQuotaReachedPreviously;

Is this to prevent downloading and asking for quota again if the user reloads the page? I think that this idea should be considered on its own merits in a separate patch.

Any such per-process state that's not saved persistently could be confusing to users of platforms that don't expose application lifetime explicitly.

> Source/WebKit/mac/WebView/WebUIDelegatePrivate.h:208
> +- (void)webView:(WebView *)sender exceededApplicationCacheOriginQuotaForSecurityOrigin:(WebSecurityOrigin *)origin spaceNeeded:(NSNumber *)spaceNeeded;

I don't see any reason to use NSNumber here. It's only needed to put numbers in plists and other collections.

But I'm not absolutely sure what you want instead - likely NSUInteger, or maybe unsigned long long to match the type Foundation uses for file sizes.

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