[webkit-reviews] review denied: [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 22874] updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 22874: updated patch
https://bugs.webkit.org/attachment.cgi?id=22874&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This looks great. I have a few questions about it.

How can we test this? I assume you did some testing already, but I'm curious
how we can prevent changes from causing regressions in this area. I'm also
thinking that the division of labor between WebCore and the underlying loader
library is getting weaker.

Some of the code here should probably be using functions from ResourceHandle;
maybe the constant "304" could be coming from there for example and also the
"can revalidate".

 195	 if (resource->resourceToRevalidate())
 196	     return;

I don't like the way this if statement reads. It's in a function named
revalidateResource it says "if there is a resource to revalidate, just return".
This shows that the names aren't quite right. I think I understand the case
where this would be true, but I'm not sure. Maybe this is a resource that
already represents a revalidation? Maybe we need a comment explaining this a
little more?

 201	 String url = resource->url(),;

The comma here looks like a syntax error. It seems that we wouldn't be able to
compile this. Also might be more efficient to use const& here.

 282	 // Equivalent of deref() for all clients
 283	 m_clients.clear();
 284	 
 285	 unsigned moveCount = clientsToMove.size();
 286	 for (unsigned n = 0; n < moveCount; ++n)
 287	     m_resourceToRevalidate->addClient(clientsToMove[n]);

Does this create a window where the clients could go away altogether or
something along those lines? Normally doing a deref before a reref is
incorrect. Maybe it's OK since it's not literally deref(). Also, it's possible
your use of "deref()" here is out of date, since Hyatt renamed the functions
here in the cache since they weren't quite ref/deref.

 292	 return !m_loading &&
(!m_response.httpHeaderField("Last-Modified").isEmpty() ||
!m_response.httpHeaderField("ETag").isEmpty());

This assumes that ETag is implemented. It's not implemented in current versions
of CFNetwork!

 48	    void setResource(CachedResource* res) {

We put the braces on a separate line. And maybe a function like this shouldn't
be inline. The general rule is that we want to inline functions that are
completely trivial, like a single assignment, or ones where there's a clear
performance win. But not others.

 57	    CachedResourceHandleBase& operator=(const
CachedResourceHandleBase&) { return *this; } 

Do we want this function to be defined at all? Who needs to call it?

 245		 String lastModified =
resourceToRevalidate->response().httpHeaderField("Last-Modified");
 246		 String eTag =
resourceToRevalidate->response().httpHeaderField("ETag");

These can be more efficient if they use const& so they don't have to ref/deref
the string.

I'm going to say review- because of that comma (at least) and because there
seem to be enough questions that at least one might cause you to change the
patch.