[webkit-reviews] review requested: [Bug 21596] WebCore::Cache should use parsed Pragma and Cache-Control headers : [Attachment 24585] Patch v2
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Oct 22 19:15:31 PDT 2008
David Kilzer (ddkilzer) <ddkilzer at webkit.org> has asked for review:
Bug 21596: WebCore::Cache should use parsed Pragma and Cache-Control headers
https://bugs.webkit.org/show_bug.cgi?id=21596
Attachment 24585: Patch v2
https://bugs.webkit.org/attachment.cgi?id=24585&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.
More information about the webkit-reviews
mailing list