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

[bugzilla-daemon at webkit.org]

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


abarth at webkit.org changed:

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




------- Comment #14 from abarth at webkit.org  2009-06-23 04:19 PDT -------
(From update of attachment 31413)
> +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.

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

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

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

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


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