[webkit-reviews] review granted: [Bug 17998] When a resource is cached locally, WebKit should follow RFC 2616 "Specific end-to-end revalidation" instead of "Unspecified end-to-end revalidation" : [Attachment 22987] more updates

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 17998: When a resource is cached locally, WebKit should follow RFC 2616
"Specific end-to-end revalidation" instead of "Unspecified end-to-end
revalidation"
https://bugs.webkit.org/show_bug.cgi?id=17998

Attachment 22987: more updates
https://bugs.webkit.org/attachment.cgi?id=22987&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Sorry, I can't remember which of these are repeating something I already asked.


 213	     CachedScript *cs = pendingScripts.first().get();

Would be nice to move the star when you're tweaking this line of code.

 225	 int delta = static_cast<int>(resource->size());

Does this really require an explicit cast?

 103	 void revalidationSucceeded(CachedResource* revalidatingResource, const
ResourceResponse& response);
 104	 void revalidationFailed(CachedResource* revalidatingResource);

Seems that you don't need the argument names here.

 111	 return (difftime(now, m_expirationDate) >= 0);

Extra parentheses on this line.

 88	void checkDelete();

Not a great name. This names it sound like we check deletion, which doesn't
make a lot of logical sense. A better name might be deleteIfPossible() or
deleteIfNotInCache(). Those are less elegant than checkDelete, but at least a
little easier to understand. There's probably an even better name.

 301	     return !cacheControl.isEmpty() &&
(cacheControl.contains("no-cache") || (isExpired() &&
cacheControl.contains("must-revalidate")));
 302	 return isExpired() || cacheControl.contains("no-cache");

I think we'd want the case-ignoring version of contains, right?

Also, is a substring match really the correct way to read this field? That
seems a little sloppy to me. What's the correct algorithm?

 134	 void setResponse(const ResourceResponse& response);
 188	 void setResourceToRevalidate(CachedResource* resource);

Should omit the argument names in these cases.

 31 void CachedResourceHandleBase::setResource(CachedResource* resource) {

Brace goes on a separate line.

 39	    operator bool() const { return get(); }

We normally don't use operator bool. I think it would be best to imitate the
technique used in our smart pointers such as RefPtr instead.

 42	    CachedResourceHandleBase() : m_resource(0) {}

We use a space in these parentheses.

 46	    void setResource(CachedResource* res);

Should omit the argument names in these cases.

None of these are serious concerns, so I'll say review+.