Do not log Extensions.InjectedScriptExecutionTime histograms if script not executed

extensions/renderer/script_injection.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/renderer/script_injection.h"
 
 #include <map>
 #include <utility>
 
 #include "base/lazy_instance.h"
@@ skipping 73 matching lines @@
   explicit TimedScriptInjectionCallback(
       base::WeakPtr<ScriptInjection> injection)
       : ScriptInjectionCallback(
             base::Bind(&TimedScriptInjectionCallback::OnCompleted,
                        base::Unretained(this))),
         injection_(injection) {}
   ~TimedScriptInjectionCallback() override {}
 
   void OnCompleted(const std::vector<v8::Local<v8::Value>>& result) {
     if (injection_) {
-      base::TimeDelta elapsed = base::TimeTicks::Now() - start_time_;
+      base::Optional<base::TimeDelta> elapsed;
+      if (!start_time_.is_null())

[Devlin, 2017/04/06 21:32:08]

Can we document this, either here or in OnJsInjectionCompleted?

// If the script will never execute (such as if the context is
// destroyed), willExecute() will not be called, but OnCompleted()
// will. Only log a time for execution if the script, in fact,
// executed.

[Kunihiko Sakamoto, 2017/04/07 01:52:50]

Done.

+        elapsed = base::TimeTicks::Now() - start_time_;
       injection_->OnJsInjectionCompleted(result, elapsed);
     }
   }
 
   void willExecute() override { start_time_ = base::TimeTicks::Now(); }
 
  private:
   base::WeakPtr<ScriptInjection> injection_;
   base::TimeTicks start_time_;
 };
@@ skipping 208 matching lines @@
       option = blink::WebLocalFrame::Synchronous;
     }
     web_frame->requestExecuteScriptInIsolatedWorld(
         world_id, &sources.front(), sources.size(), is_user_gesture, option,
         callback.release());
   }
 }
 
 void ScriptInjection::OnJsInjectionCompleted(
     const std::vector<v8::Local<v8::Value>>& results,
-    base::TimeDelta elapsed) {
+    base::Optional<base::TimeDelta> elapsed) {
   DCHECK(!did_inject_js_);
 
-  if (injection_host_->id().type() == HostID::EXTENSIONS) {
-    UMA_HISTOGRAM_TIMES("Extensions.InjectedScriptExecutionTime", elapsed);
+  if (injection_host_->id().type() == HostID::EXTENSIONS &&
+      elapsed.has_value()) {

[Devlin, 2017/04/06 21:32:08]

nit: base::Optional has an operator bool(), so this can just be
&& elapsed

[Kunihiko Sakamoto, 2017/04/07 01:52:49]

Done.

+    UMA_HISTOGRAM_TIMES("Extensions.InjectedScriptExecutionTime",
+                        elapsed.value());

[Devlin, 2017/04/06 21:32:08]

optional nit: I'd slightly prefer *elapsed here (and below), since it's a bit
shorter and more conventional, but I don't have a strong preference.

[Kunihiko Sakamoto, 2017/04/07 01:52:49]

Done.

     switch (run_location_) {
       case UserScript::DOCUMENT_START:
         UMA_HISTOGRAM_TIMES(
-            "Extensions.InjectedScriptExecutionTime.DocumentStart", elapsed);
+            "Extensions.InjectedScriptExecutionTime.DocumentStart",
+            elapsed.value());
         break;
       case UserScript::DOCUMENT_END:
         UMA_HISTOGRAM_TIMES(
-            "Extensions.InjectedScriptExecutionTime.DocumentEnd", elapsed);
+            "Extensions.InjectedScriptExecutionTime.DocumentEnd",
+            elapsed.value());
         break;
       case UserScript::DOCUMENT_IDLE:
         UMA_HISTOGRAM_TIMES(
-            "Extensions.InjectedScriptExecutionTime.DocumentIdle", elapsed);
+            "Extensions.InjectedScriptExecutionTime.DocumentIdle",
+            elapsed.value());
         break;
       default:
         break;
     }
   }
 
   bool expects_results = injector_->ExpectsResults();
   if (expects_results) {
     if (!results.empty() && !results[0].IsEmpty()) {
       // Right now, we only support returning single results (per frame).
@@ skipping 26 matching lines @@
 void ScriptInjection::InjectCss(std::set<std::string>* injected_stylesheets,
                                 size_t* num_injected_stylesheets) {
   std::vector<blink::WebString> css_sources = injector_->GetCssSources(
       run_location_, injected_stylesheets, num_injected_stylesheets);
   blink::WebLocalFrame* web_frame = render_frame_->GetWebFrame();
   for (const blink::WebString& css : css_sources)
     web_frame->document().insertStyleSheet(css);
 }
 
 }  // namespace extensions