[Webkit-unassigned] [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"

[bugzilla-daemon at webkit.org]

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


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #22987|review?                     |review+
               Flag|                            |




------- Comment #20 from darin at apple.com  2008-08-30 19:14 PDT -------
(From update of attachment 22987)
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+.


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