[webkit-reviews] review granted: [Bug 64177] ApplicationCache update should not immediately fail when reaching per-origin quota : [Attachment 100196] [PATCH] Attempt to Fix Qt Build

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted 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 100196: [PATCH] Attempt to Fix Qt Build
https://bugs.webkit.org/attachment.cgi?id=100196&action=review

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