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

[bugzilla-daemon at webkit.org]

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





------- Comment #18 from andreip at google.com  2009-07-01 17:13 PDT -------
(In reply to comment #14)
> (From update of attachment 31413 [review])
> > +ChromeClient* ApplicationCacheGroup::client()
> > +{
> > +    ASSERT(m_frame);
> > +    return m_frame->page()->chrome()->client();
> > +}
> 
> Because m_frame can be null, it might make more sense to return null if
> !m_frame.  That at least warns consumers of this API that the client might not
> be there.
>

This method is no longer needed.

> > +int64_t ApplicationCacheResource::size()
> > +{
> > +    if (!m_size) {
> 
> WebKit prefers early exit to increasing nesting depth.  So, we'd prefer:
> 
> if (m_size)
>     return m_size;
> 

Done.

> > +            m_size += (it->first.length() + it->second.length() + 2) * sizeof(UChar);
> 
> It would be good to name this magic constant 2.  I presume this is strlen(":
> ")?
> 
> > +        m_size += url().string().length() * sizeof(UChar);
> > +        m_size += sizeof(int); // response().m_httpStatusCode
> > +        m_size += response().url().string().length() * sizeof(UChar);
> > +        m_size += sizeof(unsigned); // dataId
> > +        m_size += response().mimeType().length() * sizeof(UChar);
> > +        m_size += response().textEncodingName().length() * sizeof(UChar);
> 
> This code is kind of ridiculous.  There must be a cleaner way to do this.  Do
> we really need to track the four bytes from the HTTP status code?
>

This is the code that is used to save a resource to the app cache db. It
therefore seems reasonable to also use it
for approximating the size of a resource. As discussed with Alexey on IRC, I
added a comment in ApplicationCacheStorage.cpp to make sure that anyone who
changes the code for saving a resource into the
database also changes this code.


> > +        // Returns the maximum size (in bytes) of the application caches file.
> > +        virtual int64_t maxAppCacheSize() = 0;
> 
> Why is this a method on ChromeClient?  This seems more appropriate as a value
> on the Settings object.  ChromeClient is able showing UI elements that WebCore
> can't show itself (e.g., a JavaScript alert).
> 

Removed.

> > +        // Callback invoked when the application cache gets hits the maximum size.
> > +        // The chrome client would need to take some action such as evicting some
> > +        // old caches.
> > +        // 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.
> > +        virtual void reachedMaxAppCacheSize(int64_t spaceNeeded) = 0;
> 
> Again, I don't think ChromeClient is the right place for this.  I suspect the
> FrameLoaderClient is more correct here.  Something like
> didReachMaxAppCacheSize.  You should check with Maciej to make sure you put
> this on the right client.
>

I disagree. I think this is the right place for this callback, just as the
Storage module has defined a similar callback for the situation when an origin
hits its quota. A ChromeClient implementation may wish to display some UI that
allows
the user to free some space, so having this callback here seems correct.

Thanks,
Andrei


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