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 310 matching lines @@
     } else if (isHTMLStyleElement(element)) {
       HTMLStyleElement& styleElement = toHTMLStyleElement(element);
       if (CSSStyleSheet* sheet = styleElement.sheet())
         serializeCSSStyleSheet(*sheet, KURL());
     }
   }
 }
 
 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.
   double cssStartTime = 0;
   if (!m_isSerializingCss) {
     m_isSerializingCss = true;
     cssStartTime = monotonicallyIncreasingTime();
   }
 
-  StringBuilder cssText;
-  cssText.append("@charset \"");
-  cssText.append(styleSheet.contents()->charset().lower());
-  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");
+    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 {

[Ł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) {
     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