[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





--- Comment #17 from Joseph Pecoraro <joepeck at webkit.org>  2011-07-12 14:41:22 PST ---
(From update of attachment 100196)
View in context: https://bugs.webkit.org/attachment.cgi?id=100196&action=review

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:75
>> +    , m_originQuotaReachedPreviously(false)
> 
> Maybe s/reached/exceeded/? It doesn't seem confusing in practice though.

Sure, I'll make this rename. I think its clearer.

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:604
>> +        recalculateAvailableSpaceInQuota();
> 
> Even if it's known, it could have changed due to another appcache in the same domain. That needs a FIXME comment.

Done.

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:615
>> +        cacheUpdateFailed();
> 
> I'm still very unhappy to see two "cacheUpdateFailed" calls in a row.

I removed this. I took the approach I mentioned in a previous comment and pulled the
log statement out to the call sites, and set the exceeded previously variable the only place
it was needed.

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:803
>>      // 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.

Done.

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:827
>> +        // 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.

But it is something I did in a previous patch =/. It uses an out parameter with a return bool
for succeeded/failed. Should I switch instead to returning a number, and having a bool
out param for success/failure (like parseDouble from String)?

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:913

> 
> s/encountered/exceeded/

Done.

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:940
>>              if (failureReason == ApplicationCacheStorage::TotalQuotaReached && !m_calledReachedMaxAppCacheSize) {
> 
> It's a pity that logic for total quota reached is so different from origin quota (I don't really understand what happens for global quota). Perhaps that's worth a FIXME too.

Will do, I also don't know exactly what happens here. I also don't think it should return here without
reseting the state. I added:

    // FIXME: Should this be handled more like Origin Quotas? Does this fail properly?

>> Tools/DumpRenderTree/qt/DumpRenderTreeQt.h:122
>> +    void dumpApplicationCacheQuota(QWebSecurityOrigin* origin, quint64 defaultOriginQuota, quint64 spaceNeeded);
> 
> The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]

Old code, I'll leave it since the rest of this file has similar style warnings.

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