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

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 a2258654313ad94f65350fda26b7e48f38c62b14..095ae9f1a2eb23194135d11532728f82fc599f91 100644
--- a/third_party/WebKit/Source/core/frame/FrameSerializer.cpp
+++ b/third_party/WebKit/Source/core/frame/FrameSerializer.cpp
@@ -267,7 +267,14 @@ void SerializerMarkupAccumulator::AppendRewrittenAttribute(
 
 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");
@@ -299,6 +306,8 @@ void FrameSerializer::SerializeFrame(const LocalFrame& frame) {
         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())
@@ -337,10 +346,36 @@ void FrameSerializer::SerializeFrame(const LocalFrame& frame) {
       }
     } 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,
@@ -348,12 +383,16 @@ void FrameSerializer::SerializeCSSStyleSheet(CSSStyleSheet& style_sheet,
   // 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());
@@ -478,9 +517,13 @@ void FrameSerializer::AddToResources(
 
 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());