[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"

Sun Aug 24 17:20:45 PDT 2008

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

------- Comment #18 from koivisto at iki.fi  2008-08-24 17:20 PDT -------
(In reply to comment #16)
> (From update of attachment 22874 [edit])
> 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.

I don't have good story for that. I have been running versions of this patch in
regular use over a few months but that obviously that obviously does not catch
that much. Automated HTTP tests that give code coverage should be possible but
good coverage of real world scenarios (involving potentially broken servers,
caches, etc) is very difficult. This is true for all our networking code.

I don't think division of labor is getting weaker. That overall idea is to
treat networking library cache and WebCore cache as two separate levels in HTTP
cache hierarchy. The specification is written in multiple cache levels in mind
and the interaction between levels is well defined. This allows us to implement
functionality (like ETag, below) without caring if it is implemented in other
levels.

For example if the CFNetwork level includes a cache validator to a request for
an item in its own cache we never see the resulting 304 response in WebCore
level. It gets transparently turned into 200 response.  We only get 304
responses for requests that include a validator from the WebCore level.

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

I don't like the name either. I changed it a few times and this was the best I
came up with.

The questions here is along the lines of "Is this a resource that represents a
request that includes cache validator for an existing resource".

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

Good catch. Suprisingly it compiles fine, as does

int a,;

No idea if this is a compiler bug or if it is legal C++.

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

Yeah, deref() comment is pre-rename.

CachedResources have weird destruction rules that involve multiple conditions
(CachedResources::canDelete()), and this patch adds more (for resources in
process of being revalidated) so just removing clients don't kill the objects. 

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

The implementation is entirely self contained in WebCore. We can validate
WebCore cache with ETag validators even if CFNetwork can not do it with its
cache.

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

Ok.

>  57         CachedResourceHandleBase& operator=(const
> CachedResourceHandleBase&) { return *this; } 
> 
> Do we want this function to be defined at all? Who needs to call it?

I made it private to ensure that no one is invoking the default version. The
handle went though various iterations, that helped to catch bugs at least in
some form.

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

Thanks for the 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.