[webkit-reviews] review granted: [Bug 131757] CachedResourceLoader should check redirections to reuse or not cached resources : [Attachment 230075] Fixed mac compilation

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 131757: CachedResourceLoader should check redirections to reuse or not
cached resources
https://bugs.webkit.org/show_bug.cgi?id=131757

Attachment 230075: Fixed mac compilation
https://bugs.webkit.org/attachment.cgi?id=230075&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=230075&action=review


Looks good, some style comments.

> Source/WebCore/loader/cache/CachedResource.cpp:393
> -double CachedResource::currentAge() const
> +double CachedResource::currentAge(const ResourceResponse& response, double
responseTimestamp) const

You should turn this into static standalone helper function as it doesn't use
the object fields anymore.

> Source/WebCore/loader/cache/CachedResource.cpp:405
> -double CachedResource::freshnessLifetime() const
> +double CachedResource::freshnessLifetime(const ResourceResponse& response)
const

This too.

> Source/WebCore/loader/cache/CachedResource.cpp:437
> +    m_requestedFromNetworkingLayer = true;
> +    if (!response.isNull()) {
> +	   if (m_redirectChainCacheStatus != NotCachedRedirection) {

Please use early return:

if (response.isNull())
   return;
if (m_redirectChainCacheStatus == NotCachedRedirection)
   return;
...

> Source/WebCore/loader/cache/CachedResource.cpp:814
> +bool CachedResource::redirectChainAllowsReuse() const
> +{
> +    if (m_redirectChainCacheStatus == CachedRedirection && currentTime() >
m_redirectChainEndOfValidity)
> +	   return false;
> +    return m_redirectChainCacheStatus != NotCachedRedirection;

switch (m_redirectChainCacheStatus) would make this nicer.