Do not log Extensions.InjectedScriptExecutionTime histograms if script not executed

extensions/renderer/script_injection.cc

Index: extensions/renderer/script_injection.cc
diff --git a/extensions/renderer/script_injection.cc b/extensions/renderer/script_injection.cc
index cd7da1bbbf78a4db4c6db0f5bf6ff320242802f7..64c2b5d22e5248a9b1b69c29502bc538f8a63dd6 100644
--- a/extensions/renderer/script_injection.cc
+++ b/extensions/renderer/script_injection.cc
@@ -91,7 +91,9 @@ class TimedScriptInjectionCallback : public ScriptInjectionCallback {
 
   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);
     }
   }
@@ -320,23 +322,28 @@ void ScriptInjection::InjectJs(std::set<std::string>* executing_scripts,
 
 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;