[webkit-reviews] review cancelled: [Bug 21596] WebCore::Cache should use parsed Pragma and Cache-Control headers : [Attachment 24352] Patch v1

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has cancelled David Kilzer
(ddkilzer) <ddkilzer at webkit.org>'s request for review:
Bug 21596: WebCore::Cache should use parsed Pragma and Cache-Control headers
https://bugs.webkit.org/show_bug.cgi?id=21596

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

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
(In reply to comment #2)
> (From update of attachment 24352 [edit])
> +void ResourceResponseBase::parsePragmaDirectives(PragmaDirectiveMap& result)

> const
> +{
> +    lazyInit();
> +void
> ResourceResponseBase::parseCacheControlDirectives(CacheControlDirectiveMap&
> result) const
> +{
> +    lazyInit();
> 
> Would it make sense to save the parsed PragmaDirectiveMap and
> CacheControlDirectiveMap in the ResourceResponseBase? This way they would
need
> to parsed only once.

Fixed.	Added data members to ResourceResponseBase class along with boolean
flags to tell when the maps are dirty and the headers need to be reparsed.

> +	   const String directive = directives[i].first.lower();
> +
> +	   Vector<String> directiveValues;
> +	   if ((directive == "private" || directive == "no-cache") 
> 
> This could probably use equalIgnoringCase() instead of calling lower() and
make
> directive a reference. The results already end up in a case folding hash.

Fixed.

> +PassRefPtr<StringImpl>
StringImpl::removeCharacters(CharacterMatchFunctionPtr
> findMatch)
> 
> This functions makes a copy even if nothing changes. Could that be avoided?

Yes.  Fixed.

(In reply to comment #3)
> (In reply to comment #1)
> > Created an attachment (id=24352) [edit]
> > Patch v1
> > 
> > Looking for feedback specifically on whether I should replace
> > isControlCharacter(UChar) with !isASCIIPrintable().
> 
> Spec says control characters are ASCII 0-31 and 127 (DEL).  Need to fix the
> method.

Fixed.

> > I think I should make trimToNextSeparator(const String&) inline as well.
> 
> Will do this.

Fixed.