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

[bugzilla-daemon at webkit.org]

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





--- Comment #24 from Andrei Popescu <andreip at google.com>  2009-07-14 06:50:11 PDT ---
Hi Alexey,

Thanks for the comments!

(In reply to comment #22)
> (From update of attachment 32162 [details])
> > +        * 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).
>

Done.

> > +    , 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?
>

Changed the name and added a comment that explains when this flag is set, used
and reset.

> > +    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.
> 

I agree with you. Given the size of this patch, would it be ok with you to land
it 'as is' and then fix this issue in a separate patch?


> > +            // 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.
> 

Fixed.

> > +class ChromeClientCallbackTimer: public TimerBase {
> ...
> > +    ApplicationCacheGroup* m_cacheGroup;
> 
> A comment explaining why m_cacheGroup won't be deleted before the timer fires
> would help.
>

Added.

> > +void ApplicationCacheGroup::scheduleReachedMaxAppCacheSize()
> 
> Looks like a noun is missing in this name ("callback"?).
>

Yep, added.

> > +class ChromeClientCallbackTimer;
> 
> I don't think you need a forward declaration here - a friend declaration should
> be sufficient.
>

True, removed fwd declaration.

> > +#include <sqlite3.h>
> 
> ApplicationCacheStorage shouldn't be talking to SQLite directly.

Fixed.

> >          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?
> 

It can fail if SQLite fails for whatever reason. It cannot leave the disk
database in an inconsistent state since loadCache doesn't write anything to the
database. The pattern of checking the result of loadCache() was the same
everywhere else: check the result and bail out if null. Here, the check was
missing and I think this was a simple omission so I merely fixed it to match
the existing pattern. 

Also, I think that 'continue;' is the right thing to do. We are in the cache
selection algorithm so if we cannot load a cache, we simply ignore it and move
to the next group.

> > +        // 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.
> 

Right. Changed the comment.

> > +        // 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.
>

Dropped.

> 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.

Thanks again. I have uploaded a new patch.

Andrei

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