[Webkit-unassigned] [Bug 22700] ApplicationCache should have size limit

[bugzilla-daemon at webkit.org]

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


Alexey Proskuryakov <ap at webkit.org> changed:

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




--- Comment #22 from Alexey Proskuryakov <ap at webkit.org>  2009-07-10 09:38:13 PDT ---
(From update of attachment 32162)
> +        * loader/appcache/ApplicationCache.cpp:
> +        * loader/appcache/ApplicationCache.h:
> +        Adds the ability to calculate the approximate size of an ApplicationCache object.

I think that the data member and accessor naming should make it clear how the
data relates to reality (size() is not a great name for approximate/estimated
size).

> +    , m_calledChromeClient(false)

This is a super confusing name. What method was called? Why was it called? Was
it at any point in the past, or in some time range we care about?

> +    m_frame->page()->chrome()->client()->reachedMaxAppCacheSize(cacheStorage().spaceNeeded(m_cacheBeingUpdated->size()));
> +    m_calledChromeClient = true;
> +    checkIfLoadIsComplete();

The synchronous call to client matches how it is done for database quotas, but
in the grand scheme of things, I'm not sure if WebCore should be blocked while
waiting for the UI here.

> +            // the maximum size. In such a case, m_manifestResource may be NULL, as

We do not use NULL in code, so using it in comments isn't appropriate either. I
suggest lower case "null" or 0.

> +class ChromeClientCallbackTimer: public TimerBase {
...
> +    ApplicationCacheGroup* m_cacheGroup;

A comment explaining why m_cacheGroup won't be deleted before the timer fires
would help.

> +void ApplicationCacheGroup::scheduleReachedMaxAppCacheSize()

Looks like a noun is missing in this name ("callback"?).

> +class ChromeClientCallbackTimer;

I don't think you need a forward declaration here - a friend declaration should
be sufficient.

> +#include <sqlite3.h>

ApplicationCacheStorage shouldn't be talking to SQLite directly.
>          unsigned newestCacheID = static_cast<unsigned>(statement.getColumnInt64(2));
>          RefPtr<ApplicationCache> cache = loadCache(newestCacheID);
> +        if (!cache)
> +            continue;

When can this fail? Wouldn't it mean that we left the disk database in
inconsistent state?

> +        // Callback invoked when the application cache gets hits the maximum size.

s/gets//. But also, is this comment 100% correct? I'd expect it to be called
when something doesn't fit.

> +        // spaceNeeded denotes the amount of space that would need to be freed
> +        // in order for the operation that caused the out-of-space condition to
> +        // succeed.

That part seems obvious from argument name, I'd just drop it.

This looks pretty close to being ready to me. It would be great if Anders took
a look, too - especially at ApplicationCacheStorage.cpp changes, which I didn't
study very closely.

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