[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 #100196|review?                     |review+
               Flag|                            |




--- Comment #15 from Alexey Proskuryakov <ap at webkit.org>  2011-07-12 12:04:19 PST ---
(From update of attachment 100196)
View in context: https://bugs.webkit.org/attachment.cgi?id=100196&action=review

> It is total space needed. I could improve the function's documentation and
> rename the parameter to totalSpaceNeeded or something else.

Yes, please do.

Looks good to me.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:75
> -    , m_originQuotaReached(false)
> +    , m_originQuotaReachedPreviously(false)

Maybe s/reached/exceeded/? It doesn't seem confusing in practice though.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:604
> +    if (m_availableSpaceInQuota == ApplicationCacheStorage::unknownQuota())
> +        recalculateAvailableSpaceInQuota();

Even if it's known, it could have changed due to another appcache in the same domain. That needs a FIXME comment.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:615
>          cacheUpdateFailedDueToOriginQuota();
> +        cacheUpdateFailed();

I'm still very unhappy to see two "cacheUpdateFailed" calls in a row.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:803
>      // Inform the client the origin quota has been reached,
>      // they may decide to increase the quota.

This comment doesn't need to be split into two short lines.

Perhaps it's worth mentioning that we expect quota to be increased synchronously while waiting for the call to return. At least, I was confused about that despite having learned that while reviewing the original quota code.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:827
> +    if (!cacheStorage().remainingSizeForOriginExcludingCache(m_origin.get(), m_newestCache.get(), m_availableSpaceInQuota)) {
> +        // Failed to determine what is left in the quota. Fallback to allowing anything.

It's a bad coding style violation that a function called remainingSizeForOriginExcludingCache() returns a boolean, not the remaining size. This is a pre-existing issue, and not something you did in this patch.

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

s/encountered/exceeded/

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