[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 #22874|review?                     |review-
               Flag|                            |




------- Comment #16 from darin at apple.com  2008-08-24 15:32 PDT -------
(From update of attachment 22874)
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.


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