[Offline Pages] Adding missing image/CSS detection in FrameSerializer.

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 249 matching lines @@
 
 // TODO(tiger): Right now there is no support for rewriting URLs inside CSS
 // documents which leads to bugs like <https://crbug.com/251898>. Not being
 // able to rewrite URLs inside CSS documents means that resources imported from
 // url(...) statements in CSS might not work when rewriting links for the
 // "Webpage, Complete" method of saving a page. It will take some work but it
 // needs to be done if we want to continue to support non-MHTML saved pages.
 
 FrameSerializer::FrameSerializer(Deque<SerializedResource>& resources,
                                  Delegate& delegate)
-    : resources_(&resources), is_serializing_css_(false), delegate_(delegate) {}
+    : resources_(&resources),
+      is_serializing_css_(false),
+      delegate_(delegate),
+      total_image_count_(0),
+      loaded_image_count_(0),
+      total_css_count_(0),
+      loaded_css_count_(0),
+      should_collect_problem_metric_(false) {}
 
 void FrameSerializer::SerializeFrame(const LocalFrame& frame) {
   TRACE_EVENT0("page-serialization", "FrameSerializer::serializeFrame");
   DCHECK(frame.GetDocument());
   Document& document = *frame.GetDocument();
   KURL url = document.Url();
 
   // If frame is an image document, add the image and don't continue
   if (document.IsImageDocument()) {
     ImageDocument& image_document = ToImageDocument(document);
@@ skipping 11 matching lines @@
     String text =
         SerializeNodes<EditingStrategy>(accumulator, document, kIncludeNode);
 
     CString frame_html =
         document.Encoding().Encode(text, WTF::kEntitiesForUnencodables);
     resources_->push_back(SerializedResource(
         url, document.SuggestedMIMEType(),
         SharedBuffer::Create(frame_html.data(), frame_html.length())));
   }
 
+  should_collect_problem_metric_ =
+      delegate_.ShouldCollectProblemMetric() && frame.IsMainFrame();
   for (Node* node : serialized_nodes) {
     DCHECK(node);
     if (!node->IsElementNode())
       continue;
 
     Element& element = ToElement(*node);
     // We have to process in-line style as it might contain some resources
     // (typically background images).
     if (element.IsStyledElement()) {
       RetrieveResourcesForProperties(element.InlineStyle(), document);
@@ skipping 18 matching lines @@
       }
     } else if (isHTMLLinkElement(element)) {
       HTMLLinkElement& link_element = toHTMLLinkElement(element);
       if (CSSStyleSheet* sheet = link_element.sheet()) {
         KURL url = document.CompleteURL(
             link_element.getAttribute(HTMLNames::hrefAttr));
         SerializeCSSStyleSheet(*sheet, url);
       }
     } else if (isHTMLStyleElement(element)) {
       HTMLStyleElement& style_element = toHTMLStyleElement(element);
-      if (CSSStyleSheet* sheet = style_element.sheet())
+      CSSStyleSheet* sheet = style_element.sheet();
+      if (sheet)
         SerializeCSSStyleSheet(*sheet, KURL());
     }
   }
+  if (should_collect_problem_metric_) {
+    // Report detectors through UMA.
+    // We're having exact 21 buckets for percentage because we want to have 5%
+    // in each bucket to avoid potential spikes in the distribution.
+    DCHECK_LE(loaded_image_count_, total_image_count_);
+    DEFINE_STATIC_LOCAL(
+        CustomCountHistogram, image_histogram,
+        ("PageSerialization.ProblemDetection.LoadedImagePercentage", 1, 100,
+         21));
+    image_histogram.Count(
+        static_cast<int64_t>(loaded_image_count_ * 100 / total_image_count_));
+    UMA_HISTOGRAM_COUNTS_100(
+        "PageSerialization.ProblemDetection.TotalImageCount",
+        static_cast<int64_t>(total_image_count_));
+
+    DCHECK_LE(loaded_css_count_, total_css_count_);
+    DEFINE_STATIC_LOCAL(
+        CustomCountHistogram, css_histogram,
+        ("PageSerialization.ProblemDetection.LoadedCSSPercentage", 1, 100, 21));
+    css_histogram.Count(
+        static_cast<int64_t>(loaded_image_count_ * 100 / total_image_count_));
+    UMA_HISTOGRAM_COUNTS_100("PageSerialization.ProblemDetection.TotalCSSCount",
+                             static_cast<int64_t>(total_css_count_));
+    should_collect_problem_metric_ = false;
+  }
 }
 
 void FrameSerializer::SerializeCSSStyleSheet(CSSStyleSheet& style_sheet,
                                              const KURL& url) {
   // If the URL is invalid or if it is a data URL this means that this CSS is
   // defined inline, respectively in a <style> tag or in the data URL itself.
   bool is_inline_css = !url.IsValid() || url.ProtocolIsData();
+  if (should_collect_problem_metric_)
+    total_css_count_++;
   // If this CSS is not inline then it is identifiable by its URL. So just skip
   // it if it has already been analyzed before.
   if (!is_inline_css && (resource_urls_.Contains(url) ||
                          delegate_.ShouldSkipResourceWithURL(url))) {
     return;
   }
+  if (should_collect_problem_metric_ && style_sheet.LoadCompleted())
+    loaded_css_count_++;
 
   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 css_start_time = 0;
   if (!is_serializing_css_) {
     is_serializing_css_ = true;
     css_start_time = MonotonicallyIncreasingTime();
   }
 
@@ skipping 104 matching lines @@
     DLOG(ERROR) << "No data for resource " << url.GetString();
     return;
   }
 
   resources_->push_back(SerializedResource(url, mime_type, std::move(data)));
   resource_urls_.insert(url);
 }
 
 void FrameSerializer::AddImageToResources(ImageResourceContent* image,
                                           const KURL& url) {
+  if (should_collect_problem_metric_)
+    total_image_count_++;

[Łukasz Anforowicz, 2017/06/08 16:08:45]

I wonder how the data will be calculated in case of an html with the same image
appearing twice (e.g. <img src="same-uri.jpg"> <img src="same-uri.jpg">).  AFAIK
the second occurence of the image will exit early via ShouldAddURL, and 
therefore I think this might double-count |total_image_count_|, while still
correctly counting |loaded_image_count_| (and therefore artificially reporting a
problem rate of 50% in this example). 

Does the concern above make sense?  Maybe this can be solved by moving
processing of |total_xxx_count_| below ShouldAddURL (here and for css)?

[romax, 2017/06/08 20:02:07]

Sorry for lacking knowledge about processing HTMLs... I think your concern
totally makes sense.
I've separated the check which contains ShouldAddURL since
image->ErrorOccurred() means it's loaded but failed to decode/load. I think the
code after change is a little bit ugly, but that's what I can do I guess
(without changing too much code).
And for the CSS one, seems like moving the total_count++ below the check makes
sense.

   if (!image || !image->HasImage() || image->ErrorOccurred() ||
       !ShouldAddURL(url))
     return;
+  if (should_collect_problem_metric_ && image->IsLoaded())
+    loaded_image_count_++;
 
   TRACE_EVENT2("page-serialization", "FrameSerializer::addImageToResources",
                "type", "image", "url", url.ElidedString().Utf8().data());
   double image_start_time = MonotonicallyIncreasingTime();
 
   RefPtr<const SharedBuffer> data = image->GetImage()->Data();
   AddToResources(image->GetResponse().MimeType(),
                  image->HasCacheControlNoStoreHeader()
                      ? kHasCacheControlNoStoreHeader
                      : kNoCacheControlNoStoreHeader,
@@ skipping 85 matching lines @@
     emits_minus = ch == '-';
     builder.Append(ch);
   }
   CString escaped_url = builder.ToString().Ascii();
   return String::Format("saved from url=(%04d)%s",
                         static_cast<int>(escaped_url.length()),
                         escaped_url.data());
 }
 
 }  // namespace blink