[webkit-reviews] review denied: [Bug 64177] ApplicationCache update should not immediately fail when reaching per-origin quota : [Attachment 100138] [PATCH] Proposed Patch for Landing

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jul 8 11:41:19 PDT 2011


Alexey Proskuryakov <ap at webkit.org> has denied Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 64177: ApplicationCache update should not immediately fail when reaching
per-origin quota
https://bugs.webkit.org/show_bug.cgi?id=64177

Attachment 100138: [PATCH] Proposed Patch for Landing
https://bugs.webkit.org/attachment.cgi?id=100138&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.


More information about the webkit-reviews mailing list