[webkit-reviews] review denied: [Bug 105667] Enable reuse of cached main resources : [Attachment 182218] + test

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 105667: Enable reuse of cached main resources
https://bugs.webkit.org/show_bug.cgi?id=105667

Attachment 182218: + test
https://bugs.webkit.org/attachment.cgi?id=182218&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=182218&action=review


review- because of the bad cast I found

> Source/WebCore/loader/FrameLoader.cpp:2890
> +    // Main resource delegate messages are synthesized in the same way as
main resource SubstituteData loads.

For someone not as expert, this comment is a little oblique, like haiku. I
would say something more like this:

    // Main resource delegate messages are synthesized, so we must not send
them here.

Better to be specific about the function or class that does the synthesizing.

I’m not sure what the relevance of the SubstituteData comparison is, because
this function doesn’t have code to handle SubstituteData as a special case.
It’s sort of a riddle to compare the two here without explaining why that
comparison is relevant to this code.

> Source/WebCore/loader/MainResourceLoader.cpp:393
> +    bool loadFallback =
documentLoader()->applicationCacheHost()->maybeLoadFallbackForMainResponse(requ
est(), r);

This should be named didLoadFallback. A variable named loadFallback would hold
“a fallback”, not a boolean that indicates whether fallback occurred.

> Source/WebCore/loader/MainResourceLoader.cpp:397
> +    if (r.appCacheID())
> +	   shouldRemoveResourceFromCache = true;

Seems like this needs a why comment.

> Source/WebCore/loader/MainResourceLoader.cpp:401
> +    // The memory cache doesn't understand the application cache or its
caching rules. So if a main resource is served
> +    // from the application cache, ensure we don't save the result for
future use.

Is this comment specific to the Chromium code, or is “did load fallback” also
an implementation detail of the application cache?

> Source/WebCore/loader/MainResourceLoader.cpp:573
> +    // If the document specified an application cache manifest, it's risky
to store in the memory cache
> +    // and deny the appcache the chance to intercept it in the future, so
remove from the memory cache.

Great comment, except for the use of the word “risky”. It seems this is either
correct or incorrect, not “risky”. We don’t want to treat our software as if
it’s a game of chance.

> Source/WebCore/loader/MainResourceLoader.cpp:705
> +    if (!loader()) {
> +	   m_substituteDataLoadIdentifier =
m_documentLoader->frame()->page()->progress()->createUniqueIdentifier();
> +	  
frameLoader()->notifier()->assignIdentifierToInitialRequest(m_substituteDataLoa
dIdentifier, documentLoader(), request);
> +	   frameLoader()->notifier()->dispatchWillSendRequest(documentLoader(),
m_substituteDataLoadIdentifier, request, ResourceResponse());
> +    }

Very enigmatic code. Why does not having a loader indicate we are loading
substitute data? Seems like a why comment is needed.

> Source/WebCore/loader/MainResourceLoader.cpp:714
>      if (loader())
>	   request = loader()->originalRequest();
> +    if (equalIgnoringFragmentIdentifier(initialRequest.url(),
request.url()))
> +	   request.setURL(initialRequest.url());

The comment talks about a task, but then there are two sets of code that both
seem to be trying to accomplish the task in two different ways. Why do we have
to do both things? Is there a way to make that obvious, perhaps with more
comment?

> Source/WebCore/loader/cache/CachedRawResource.h:72
> +    Vector<ResourceRequest> m_redirectChainRequests;
> +    Vector<ResourceResponse> m_redirectChainResponses;

Vectors have quite a bit of overhead. I am worried this could have noticeable
memory impact; maybe I’m mistaken. Is there a way we can make this more
efficient? For example, we could have a Vector of structures containing
request/response pairs instead of two vectors, which would save half of the
overhead. Generally speaking, I’m worried about storing quite a bit more data
here.

But maybe CachedRawResource isn’t used as often as I think it is?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:544
> +    if ((existingResource->type() == CachedResource::MainResource ||
existingResource->type() == CachedResource::RawResource) &&
!static_cast<CachedRawResource*>(existingResource)->canReuse(request))
>	   return Reload;

This casts existingResource to CachedRawResource even when the type is
CachedResource::MainResource. Bad cast!