[webkit-reviews] review requested: [Bug 21493] Resources added to WebCore::Cache which should never be cached : [Attachment 24338] Patch v2

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has asked  for review:
Bug 21493: Resources added to WebCore::Cache which should never be cached
https://bugs.webkit.org/show_bug.cgi?id=21493

Attachment 24338: Patch v2
https://bugs.webkit.org/attachment.cgi?id=24338&action=edit

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
(In reply to comment #5)
> (From update of attachment 24214 [edit])
> +    CacheControlDirectiveMap directives =
m_response.cacheControlDirectives();
> This will copy the CacheControlDirectiveMap.	Instead, passing a
> CacheControlDirectiveMap to build up would be more efficient.

Fixed.

> +    CacheControlDirectiveMap cacheControlDirectives =
> m_response.cacheControlDirectives();
> Same here.

Fixed.

> +	   Vector<pair<String, String> > directives =
> parseCacheHeader(pragmaHeader);
> This will also cause a copy of the Vector.  There are a few more of these as
> well.

All fixed.

> +	   for (unsigned i = 0, max = directives.size(); i < max; ++i)
> This is a bit awkward.  We usually put the max assignment on the line above.

I prefer this (when it doesn't make the line insanely long) since it scopes the
max variable to the loop, but I changed to declare max outside the loop.

> +	       result.set(directives[i].first.lower(), directives[i].second);
> Instead of calling lower, perhaps a CaseFoldingHash would be better for this.


Fixed. Thanks for the suggestion!

> +    static Vector<pair<String, String> > parseCacheHeader(const String&);
> +    static Vector<String> parseCacheControlDirectiveValues(const String&);
> These don't need to be declared in the header.  Marking the function as
static
> in the c++ will give them internal linkage.

Fixed to be declared locally within the .cpp source file.

(In reply to comment #6)
> I need to rework the parser--it doesn't follow the spec (as white space and
> other characters are included as a separator, not just a comma).

The rest of the changes were to clean up the parser, including some new methods
added to String and StringImpl.