[webkit-reviews] review denied: [Bug 22700] ApplicationCache should have size limit : [Attachment 31413] New proposal to limit the maximum size of the application cache (take 6)

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Andrei Popescu <andreip at google.com>'s
request for review:
Bug 22700: ApplicationCache should have size limit
https://bugs.webkit.org/show_bug.cgi?id=22700

Attachment 31413: New proposal to limit the maximum size of the application
cache (take 6)
https://bugs.webkit.org/attachment.cgi?id=31413&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
> +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.