Bug 21596 – WebCore::Cache should use parsed Pragma and Cache-Control headers

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Currently the WebCore::Cache uses substring searches of Cache-Control headers to search for cache directives.  Instead, it should be parsing the headers and then checking for directives in the parsed structure.

This work was done under the auspices of Bug 21493, but is being moved to a separate bug for independent review.

[David Kilzer (:ddkilzer)]

Created attachment 24352 [details]
Patch v1

Looking for feedback specifically on whether I should replace isControlCharacter(UChar) with !isASCIIPrintable().

I think I should make trimToNextSeparator(const String&) inline as well.

[Antti Koivisto]

Comment on attachment 24352 [details]
Patch v1

+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.

+        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.

+PassRefPtr<StringImpl> StringImpl::removeCharacters(CharacterMatchFunctionPtr findMatch)

This functions makes a copy even if nothing changes. Could that be avoided?

[David Kilzer (:ddkilzer)]

(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.

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

Will do this.

[David Kilzer (:ddkilzer)]

Created attachment 24585 [details]
Patch v2

(In reply to comment #2)
> (From update of attachment 24352 [details] [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.

[Antti Koivisto]

Comment on attachment 24585 [details]
Patch v2

Looks good, r=me. One minor comment:

> +PassRefPtr<StringImpl> StringImpl::removeCharacters(CharacterMatchFunctionPtr findMatch)
> +{
> +    StringBuffer data(m_length);
> +
> +    const UChar* from = m_data;
> +    const UChar* fromend = from + m_length;
> +    unsigned outc = 0;
> +
> +    UChar* to = data.characters();
> +
> +    while (true) {
> +        while (from != fromend && findMatch(*from))
> +            from++;
> +        while (from != fromend && !findMatch(*from))
> +            to[outc++] = *from++;
> +        if (from == fromend)
> +            break;
> +    }
> +
> +    if (outc == m_length)
> +        return this;

It would be more optimal to allocate the buffer when the first removable character is found. That would avoid unnecessary malloc/free in no removals case.

[David Kilzer (:ddkilzer)]

Created attachment 24927 [details]
Patch to address Bug 21596 Comment #5

Applies on top of Attachment #24585 [details].

[David Kilzer (:ddkilzer)]

$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/loader/Cache.cpp
	M	WebCore/loader/Cache.h
	M	WebCore/loader/CachedResource.cpp
	M	WebCore/loader/loader.cpp
	M	WebCore/platform/network/ResourceResponseBase.cpp
	M	WebCore/platform/network/ResourceResponseBase.h
	M	WebCore/platform/text/PlatformString.h
	M	WebCore/platform/text/String.cpp
	M	WebCore/platform/text/StringImpl.cpp
	M	WebCore/platform/text/StringImpl.h
Committed r38145