[webkit-reviews] review granted: [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 25055] updated WebCore patch

[bugzilla-daemon at webkit.org]

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +    CachePolicy cachePolicy = this->cachePolicy();
> +    if (cachePolicy == CachePolicyVerify || cachePolicy == CachePolicyCache)
{
> +	   CachedResource* existing =
cache()->resourceForURL(fullURL.string());
> +	   if (existing && !existing->isPreloaded() &&
existing->mustRevalidate(cachePolicy)) {
> +	       cache()->revalidateResource(existing, this);
> +	       m_reloadedURLs.add(fullURL.string());
> +	   }
> +	   return;
> +    }
> +    if (cachePolicy == CachePolicyReload || cachePolicy ==
CachePolicyRevalidate) {
> +	   CachedResource* existing =
cache()->resourceForURL(fullURL.string());
> +	   if (existing && !existing->isPreloaded()) {
> +	       if (cachePolicy == CachePolicyReload)
> +		   cache()->remove(existing);
> +	       else
> +		   cache()->revalidateResource(existing, this);
> +	       m_reloadedURLs.add(fullURL.string());
> +	   }
>      }

The above looks like it should be a switch statement.

> -    ScheduledRedirection(double redirectDelay, const String& redirectURL,
bool redirectLockHistory, bool userGesture)
> +    ScheduledRedirection(double redirectDelay, const String& redirectURL,
bool redirectLockHistory, bool userGesture, bool refresh)

It's a little confusing to have both the term "reload" and the term "refresh"
used.

> +    ResourceRequest request(url, referrer,  refresh ?
ReloadIgnoringCacheData : UseProtocolCachePolicy);

Two spaces here after the comma.

> -	   scheduleRedirection(new ScheduledRedirection(delay, url, delay <= 1,
false));
> +	   scheduleRedirection(new ScheduledRedirection(delay, url, delay <= 1,
false, false));

Yuck, I hate booleans. We should really use enums instead so we can see what
"false" means.

> +    // FIXME: We don't have a mechanism to revalidate the main resource
without reloadin at the moment.

Typo here. Need a "g" on the end of "reloading".

> -	   void changeLocation(const String& url, const String& referrer, bool
lockHistory = true, bool userGesture = false);
> -	   void changeLocation(const KURL&, const String& referrer, bool
lockHistory = true, bool userGesture = false);
> +	   void changeLocation(const String& url, const String& referrer, bool
lockHistory = true, bool userGesture = false, bool refresh = false);
> +	   void changeLocation(const KURL&, const String& referrer, bool
lockHistory = true, bool userGesture = false, bool refresh = false);

You added the "= false" default here, but you also added the false boolean
explicitly to some call sites. I think you should omit it at the call sites.

r=me