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

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

 /*
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 318 matching lines @@
 void FrameSerializer::serializeCSSStyleSheet(CSSStyleSheet& styleSheet,
                                              const KURL& url) {
   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.
   double cssStartTime = 0;
   if (!m_isSerializingCss) {
     m_isSerializingCss = true;
     cssStartTime = monotonicallyIncreasingTime();
   }
+  bool willAppendResource = shouldAddURL(url);
 
-  StringBuilder cssText;
-  cssText.append("@charset \"");
-  cssText.append(styleSheet.contents()->charset().lower());
-  cssText.append("\";\n\n");
+  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");
+    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();
     CString text =
         textEncoding.encode(textString, WTF::CSSEncodedEntitiesForUnencodables);
     m_resources->append(
         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) {
     m_isSerializingCss = false;
     DEFINE_STATIC_LOCAL(CustomCountHistogram, cssHistogram,
                         ("PageSerialization.SerializationTime.CSSElement", 0,
                          maxSerializationTimeUmaMicroseconds, 50));
     cssHistogram.count(
         static_cast<int64_t>((monotonicallyIncreasingTime() - cssStartTime) *
                              secondsToMicroseconds));
@@ skipping 163 matching lines @@
     emitsMinus = ch == '-';
     builder.append(ch);
   }
   CString escapedUrl = builder.toString().ascii();
   return String::format("saved from url=(%04d)%s",
                         static_cast<int>(escapedUrl.length()),
                         escapedUrl.data());
 }
 
 }  // namespace blink