[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..684c4c8e12b3996000f7bacc515226d6722e6507 100644
--- a/third_party/WebKit/Source/core/frame/FrameSerializer.cpp
+++ b/third_party/WebKit/Source/core/frame/FrameSerializer.cpp
@@ -299,6 +299,12 @@ void FrameSerializer::SerializeFrame(const LocalFrame& frame) {
         SharedBuffer::Create(frame_html.data(), frame_html.length())));
   }
 
+  int total_image_count = 0;
+  int loaded_image_count = 0;
+  int total_css_count = 0;
+  int loaded_css_count = 0;
+  bool should_collect_problem_metric =
+      delegate_.ShouldCollectProblemMetric() && frame.IsMainFrame();

[Łukasz Anforowicz, 2017/06/07 21:11:35]

You are obviously in a better position than me to decide whether you need data
for subframes, but it seems weird to me that we don't care if a CSS or image is
missing in a subframe.  FWIW, I've also commented in the design doc.

[romax, 2017/06/08 02:25:22]

I think it was mainly because in the offline page cases we care more about main
frame, and the subframes/iframes are mostly useless/ads, as commented by
petewil@ in the doc. So i'd like to keep the main frame check. Is it reasonable
to you?

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

Sounds good.  One final thought - how would the names of the metrics look like
if (hypothetically) we wanted to add tracking of 1) subframe problems or 2)
overall problems?  One option would be to have
PageSerialization.ProblemDetection.TotalImageCount.AllFrames, and
PageSerialization.ProblemDetection.TotalImageCount.Subframes in the future. 
Another option would be to insert "MainFrame" substring somewhere in the name of
the metrics being introduced in the current CL (e.g. maybe
PageSerialization.ProblemDetection.MainFrame.TotalImageCount?).

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

Adding suffixes sounds good to me, and it is something that can be added later
when we decide to record counting for sub frames. Other than ordering there
doesn't seem to have much difference. Or do you want me to add the suffix right
now?

[Łukasz Anforowicz, 2017/06/08 20:48:53]

I don't have a strong opinion - I am fine with whatever approach works for you.

   for (Node* node : serialized_nodes) {
     DCHECK(node);
     if (!node->IsElementNode())
@@ -317,6 +323,11 @@ void FrameSerializer::SerializeFrame(const LocalFrame& frame) {
       HTMLImageElement& image_element = toHTMLImageElement(element);
       KURL url =
           document.CompleteURL(image_element.getAttribute(HTMLNames::srcAttr));
+      if (should_collect_problem_metric) {
+        total_image_count++;
+        if (image_element.complete())

[Łukasz Anforowicz, 2017/06/07 21:11:35]

I see that FrameSerializer::AddImageToResources checks whether to serialize the
image, by looking at:
|!image || !image->HasImage() || image->ErrorOccurred() || !ShouldAddURL(url)|. 
Will HTMLImageElement::complete always be in-sync with the checks done by
AddImageToResources?  If not, then maybe we can consider moving the counting
logic to the body of AddImageToResources?

(my working mental model for these metrics is that we want to count how many
MHTML parts containing images we generate VS how many such parts we would have
generated if we could wait for all images to finish loading;  please shout if
this is not the right model).

[romax, 2017/06/08 02:25:22]

Done.
Your mental model is correct. And I wasn't sure if ImageElement::complete() is
the right choice. I think moving the logic into the sub-function definitely
makes more sense.

+          loaded_image_count++;
+      }

[Łukasz Anforowicz, 2017/06/07 21:11:35]

Have you thought about avoiding counting resources skipped by
|delegate_.ShouldSkipResourceWithURL(...)|?  Could you please comment on that? 
I think that if we start gathering data for both main frame and for subframes,
then avoiding counting of skipping resources would be required to avoid
double-counting resources that are included from more than 1 frame.

[romax, 2017/06/08 02:25:22]

I didn't think about it before seeing comments. And it seems like after moving
the count logic into AddImageToResources and SerializeCSSStyleSheet, the
resources which should be skipped will not be counted, so I think we're good in
this case.

       ImageResourceContent* cached_image = image_element.CachedImage();
       AddImageToResources(cached_image, url);
     } else if (isHTMLInputElement(element)) {
@@ -337,10 +348,35 @@ 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) {
+        total_css_count++;
+        if (sheet && sheet->IsLoading())
+          loaded_css_count++;

[Łukasz Anforowicz, 2017/06/07 21:11:35]

"loading" vs "loaded" seems out of sync - can you double-check and confirm that
this shouldn't be |!sheet->IsLoading()|?

Also - I assume that you've manually tested the new metrics, as described in
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...
?

[romax, 2017/06/08 02:25:22]

Done.

+      }
     }
   }
+  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);
+    UMA_HISTOGRAM_EXACT_LINEAR(
+        "PageSerialization.ProblemDetection.LoadedImagePercentage",
+        static_cast<int64_t>(loaded_image_count * 100 / total_image_count), 21);

[Steven Holte, 2017/06/07 22:36:42]

You are using 21 buckets, but these buckets are not 5% wide, you are just going
to be recording everything over 20% into the overflow bucket, which doesn't seem
like what you want.  To get 5% wide buckets, you probably need to use
base::LinearHistogram::FactoryGet, or UMA_HISTOGRAM_CUSTOM_ENUMERATION.

[romax, 2017/06/08 02:25:22]

Done.
Changed to CustomCountHistogram in Blink, since classes in base:: cannot be used
directly in Blink::Core. However i'm not sure if the change will split the
bucket into 5% ones. Otherwise I'll have to either change to 1% buckets using
HISTOGRAM_PERCENTAGE, or adding the linear histograms in Blink. Am I missing
anything?

[Steven Holte, 2017/06/08 20:43:00]

CustomCountHistogram is also not going to give you 5% wide histograms.  It
base::Histogram, which uses exponential spacing, instead of linear spacing. 
Another option is to just record the ventiles instead of percentiles with
EXACT_LINEAR (*20 instead of *100), but either of the options you suggested seem
fine to me too.

[romax, 2017/06/08 23:30:46]

I've added the LinearHistogram in Webkit/Platforms, so i'm using the linear
histogram with 21 buckets(min/max: 1/100) which I think is good for the use
case.

+    UMA_HISTOGRAM_COUNTS_100(
+        "PageSerialization.ProblemDetection.TotalImageCount",
+        static_cast<int64_t>(total_image_count));
+
+    DCHECK_LE(loaded_css_count, total_css_count);
+    UMA_HISTOGRAM_EXACT_LINEAR(
+        "PageSerialization.ProblemDetection.LoadedCSSPercentage",
+        static_cast<int64_t>(loaded_css_count * 100 / total_css_count), 21);
+    UMA_HISTOGRAM_COUNTS_100("PageSerialization.ProblemDetection.TotalCSSCount",
+                             static_cast<int64_t>(total_css_count));
+  }
 }
 
 void FrameSerializer::SerializeCSSStyleSheet(CSSStyleSheet& style_sheet,