MHTML saving optimization: don't generate CSS code that will be discarded.

third_party/WebKit/Source/core/frame/FrameSerializer.cpp

Index: third_party/WebKit/Source/core/frame/FrameSerializer.cpp
diff --git a/third_party/WebKit/Source/core/frame/FrameSerializer.cpp b/third_party/WebKit/Source/core/frame/FrameSerializer.cpp
index d1bf4cba20e6f4314bd41c492e9f2a175b5d358f..49f7d8f5549f08fbb612211ae642d9df9b95505d 100644
--- a/third_party/WebKit/Source/core/frame/FrameSerializer.cpp
+++ b/third_party/WebKit/Source/core/frame/FrameSerializer.cpp
@@ -328,6 +328,14 @@ void FrameSerializer::serializeFrame(const LocalFrame& frame) {
 
 void FrameSerializer::serializeCSSStyleSheet(CSSStyleSheet& styleSheet,
                                              const KURL& url) {
+  bool acceptableUrl = url.isValid() && !url.protocolIsData();

[Łukasz Anforowicz, 2016/10/07 19:07:48]

nit: Maybe add a comment what is the meaning of |acceptableUrl| OR maybe rename
it to isExternalCssResource or isNotInlineStyle?

[carlosk, 2016/10/07 21:54:24]

Done.

+  // If this CSS is identifiable by its URL and it has already been analyzed
+  // before then just skip it.
+  if (acceptableUrl && (m_resourceURLs.contains(url) ||
+                        m_delegate.shouldSkipResourceWithURL(url))) {
+    return;
+  }
+
   TRACE_EVENT2("page-serialization", "FrameSerializer::serializeCSSStyleSheet",
                "type", "CSS", "url", url.elidedString().utf8().data());
   // Only report UMA metric if this is not a reentrant CSS serialization call.
@@ -337,25 +345,26 @@ void FrameSerializer::serializeCSSStyleSheet(CSSStyleSheet& styleSheet,
     cssStartTime = monotonicallyIncreasingTime();
   }
 
-  StringBuilder cssText;
-  cssText.append("@charset \"");
-  cssText.append(styleSheet.contents()->charset().lower());
-  cssText.append("\";\n\n");
-
-  for (unsigned i = 0; i < styleSheet.length(); ++i) {
-    CSSRule* rule = styleSheet.item(i);
-    String itemText = rule->cssText();
-    if (!itemText.isEmpty()) {
-      cssText.append(itemText);
-      if (i < styleSheet.length() - 1)
-        cssText.append("\n\n");
-    }
+  if (acceptableUrl) {
+    StringBuilder cssText;
+    cssText.append("@charset \"");
+    cssText.append(styleSheet.contents()->charset().lower());
+    cssText.append("\";\n\n");
+
+    for (unsigned i = 0; i < styleSheet.length(); ++i) {
+      CSSRule* rule = styleSheet.item(i);
+      String itemText = rule->cssText();
+      if (!itemText.isEmpty()) {
+        cssText.append(itemText);
+        if (i < styleSheet.length() - 1)
+          cssText.append("\n\n");
+      }
 
-    // Some rules have resources associated with them that we need to retrieve.
-    serializeCSSRule(rule);
-  }
+      // Some rules have resources associated with them that we need to
+      // retrieve.
+      serializeCSSRule(rule);
+    }
 
-  if (shouldAddURL(url)) {
     WTF::TextEncoding textEncoding(styleSheet.contents()->charset());
     ASSERT(textEncoding.isValid());
     String textString = cssText.toString();
@@ -365,6 +374,10 @@ void FrameSerializer::serializeCSSStyleSheet(CSSStyleSheet& styleSheet,
         SerializedResource(url, String("text/css"),
                            SharedBuffer::create(text.data(), text.length())));
     m_resourceURLs.add(url);
+  } else {

[Łukasz Anforowicz, 2016/10/07 19:07:48]

nit: Maybe code would be cleaner if the loop below would execute unconditionally
(i.e. if the loop would be moved out of "else" body / if the "else" part of "if"
would go away + avoid calling |serializeCSSRule(rule)| in the main "if" body). 
This might also mean no need for a comment explaining why the loop is needed in
the "else" part.  WDYT?

[carlosk, 2016/10/07 21:54:24]

I considered that but thought it would make the method look more confusing.
There is already a good number of if blocks in it and this change would increase
it by two: one inside the for-loop and another following it (we would need 3 in
total for not generating CSS code).

These checks would even make the method longer (actual code lines) when compared
to repeating the loop here.

So I'm leaving this one as is.

+    // Even if no text is needed, associated resources must be fetched.

[Łukasz Anforowicz, 2016/10/07 19:07:48]

nit: I am not sure if this comment will be easy to understand for other
maintainers.  Maybe expand into: "We need to serialize subresources of CSS, even
if the CSS resource itself doesn't need to be serialized (e.g. in cases when
this CSS comes from an inline <style> tag or from a data: URI)".

[carlosk, 2016/10/07 21:54:24]

Indeed, this was unclear. I improved the overall comments of the method
including this one. Done.

+    for (unsigned i = 0; i < styleSheet.length(); ++i)
+      serializeCSSRule(styleSheet.item(i));
   }
 
   if (cssStartTime != 0) {