CSSStyleSheetResource should cache decoded text instead of raw bytes

third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp

 /*
     Copyright (C) 1998 Lars Knoll (knoll@mpi-hd.mpg.de)
     Copyright (C) 2001 Dirk Mueller (mueller@kde.org)
     Copyright (C) 2002 Waldo Bastian (bastian@kde.org)
     Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
     Copyright (C) 2004, 2005, 2006 Apple Computer, Inc.
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
     License as published by the Free Software Foundation; either
@@ skipping 72 matching lines @@
     Resource::didAddClient(c);
     if (m_didNotifyFirstData)
         static_cast<StyleSheetResourceClient*>(c)->didAppendFirstData(this);
 
     // |c| might be removed in didAppendFirstData, so ensure it is still a
     // client.
     if (hasClient(c) && !isLoading())
         static_cast<StyleSheetResourceClient*>(c)->setCSSStyleSheet(resourceRequest().url(), response().url(), encoding(), this);
 }
 
-const String CSSStyleSheetResource::sheetText(MIMETypeCheck mimeTypeCheck) const
+const String CSSStyleSheetResource::sheetText(MIMETypeCheck mimeTypeCheck)
 {
-    if (!data() || data()->isEmpty() || !canUseSheet(mimeTypeCheck))
+    if (!canUseSheet(mimeTypeCheck))
         return String();
 
+    // Only cache decoded sheet text when loading is complete.
+    if (isLoading())
+        return decodedText();
+
+    // Use cached decoded sheet text when available.
     if (!m_decodedSheetText.isNull())
         return m_decodedSheetText;
 
-    // Don't cache the decoded text, regenerating is cheap and it can use quite a bit of memory
-    return decodedText();
+    if (!data() || data()->isEmpty())
+        return String();
+
+    m_decodedSheetText = decodedText();
+    clearData();
+    return m_decodedSheetText;

[hiroshige, 2016/08/31 07:30:25]

IIUC, the Lines 106--111 are executed only in sheetText() called in
checkNotify().
How about splitting out this block to a new method and making checkNotify() to
call the new method, and keeping sheetText() const?
This will clarify when |m_decodedSheetText| is set, and also make this CL much
smaller.

[kouhei (in TOK), 2016/08/31 10:27:43]

Done.

 }
 
 void CSSStyleSheetResource::appendData(const char* data, size_t length)
 {
     Resource::appendData(data, length);
     if (m_didNotifyFirstData)
         return;
     ResourceClientWalker<StyleSheetResourceClient> w(clients());
     while (StyleSheetResourceClient* c = w.next())
         c->didAppendFirstData(this);
     m_didNotifyFirstData = true;
 }
 
 void CSSStyleSheetResource::checkNotify()
 {
     // Decode the data to find out the encoding and keep the sheet text around during checkNotify()
-    if (data())
-        m_decodedSheetText = decodedText();
+    sheetText();

[hiroshige, 2016/08/31 07:30:25]

I think we call sheetText() here to ensure |m_data| is always null after this
point.
Please add a comment (perhaps somewhere in CssStyleSheetResource.h) about the
lifetime of |m_data| and |m_decodedSheetText|.

[kouhei (in TOK), 2016/08/31 10:27:43]

Done.

 
     ResourceClientWalker<StyleSheetResourceClient> w(clients());
     while (StyleSheetResourceClient* c = w.next())
         c->setCSSStyleSheet(resourceRequest().url(), response().url(), encoding(), this);
-    // Clear the decoded text as it is unlikely to be needed immediately again and is cheap to regenerate.
-    m_decodedSheetText = String();
 }
 
 void CSSStyleSheetResource::destroyDecodedDataIfPossible()
 {
     if (!m_parsedStyleSheetCache)
         return;
 
     setParsedStyleSheetCache(nullptr);
     setDecodedSize(0);
 }
@@ skipping 45 matching lines @@
         // This stylesheet resource did conflict with another resource and was
         // not added to the cache.
         setParsedStyleSheetCache(nullptr);
         return;
     }
     setParsedStyleSheetCache(sheet);
     setDecodedSize(m_parsedStyleSheetCache->estimatedSizeInBytes());
 }
 
 } // namespace blink