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..a028cf652fbf8eec3cdacd420f5ecdb5a60350a9 100644
--- a/third_party/WebKit/Source/core/frame/FrameSerializer.cpp
+++ b/third_party/WebKit/Source/core/frame/FrameSerializer.cpp
@@ -336,26 +336,28 @@ void FrameSerializer::serializeCSSStyleSheet(CSSStyleSheet& styleSheet,
     m_isSerializingCss = true;
     cssStartTime = monotonicallyIncreasingTime();
   }
+  bool willAppendResource = shouldAddURL(url);
+
+  if (willAppendResource) {
+    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");
+      }
 
-  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 +367,10 @@ void FrameSerializer::serializeCSSStyleSheet(CSSStyleSheet& styleSheet,
         SerializedResource(url, String("text/css"),
                            SharedBuffer::create(text.data(), text.length())));
     m_resourceURLs.add(url);
+  } else {
+    // Even if no text is needed, associated resources must be fetched.

[Łukasz Anforowicz, 2016/10/06 17:21:55]

Today if 2 frames serialize CSS with the same URIs, then the second one will be
assumed to be identical to the first one (and therefore we won't serialize it
again).  "assumed to be identical" should also cover which resources are
associated with the CSS, right?  In other words - if identical-URI means
identical-CSS (including associated resources), then we can just return early,
right?

Did I get the above right?  Or are there still cases where we might need to
serialize CSS's subresources, despite the fact that a CSS with the same URI was
already serialized in a different frame?

[carlosk, 2016/10/06 18:07:24]

I think the process you are describing is the combination of checking with the
frame-local resource URIs stored in |m_resourceURLs| and URIs collected from
previously serialized resources from other frames checked by
|m_delegate.shouldSkipResourceWithURL| (which auto-populates itself at each
check and is shared future renderers). Both of these happen inside
|shouldAddURL| and in a few other places that seem to cover all cases where CSS
resources would be serialized again. 

There tests in FrameSerializerTest that try to identify issues with this logic.

My changes shouldn't affect this mechanism.
Now I don't know the answer for this question. I am not an expert in CSS and
even less in all the crazy ways it is used in the Web nowadays. I can only say
that the tests I mentioned above seem to be correct and would catch regressions
in the covered use cases.

[carlosk, 2016/10/07 18:47:29]

After an offline talk I understood lukasza@'s suggestion of completely avoiding
re-analyzing an already visited CSS. This latest patch tries to do that.

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