Don't collect a log per peerconnection if webrtc-internals isn't open.

content/browser/webrtc/webrtc_internals.cc

Index: content/browser/webrtc/webrtc_internals.cc
diff --git a/content/browser/webrtc/webrtc_internals.cc b/content/browser/webrtc/webrtc_internals.cc
index dee0c97f6e02867485153ec9df98751b36c5c7cb..fc5c9459b95f478052ea93f806e5e7927e6f7335 100644
--- a/content/browser/webrtc/webrtc_internals.cc
+++ b/content/browser/webrtc/webrtc_internals.cc
@@ -35,20 +35,26 @@ namespace content {
 
 namespace {
 
-static base::LazyInstance<WebRTCInternals>::Leaky g_webrtc_internals =
+base::LazyInstance<WebRTCInternals>::Leaky g_webrtc_internals =
     LAZY_INSTANCE_INITIALIZER;
 
 // Makes sure that |dict| has a ListValue under path "log".
-static base::ListValue* EnsureLogList(base::DictionaryValue* dict) {
+base::ListValue* EnsureLogList(base::DictionaryValue* dict) {
   base::ListValue* log = NULL;
   if (!dict->GetList("log", &log)) {
     log = new base::ListValue();
-    if (log)
-      dict->Set("log", log);
+    dict->Set("log", log);
   }
   return log;
 }
 
+// Removes the log entry associated with a given record.
+void FreeLogList(base::Value* value) {
+  DCHECK(value->IsType(base::Value::Type::DICTIONARY));
+  auto* dict = static_cast<base::DictionaryValue*>(value);

[Henrik Grunell, 2017/03/03 12:40:09]

Not being particularly familiar with base::Value, is this really how one would
get the value of the right type? Just doesn't seem very elegant, but if it's the
way then fine.

[tommi (sloooow) - chröme, 2017/03/03 14:15:14]

Yes, it's a polymorphic design with an enum that points to what actual type is
used (comparable to a VARIANT).
E.g. look at static_cast<>s here:
https://cs.chromium.org/chromium/src/base/values.cc?type=cs&sq=package:chromium

[Henrik Grunell, 2017/03/06 11:49:24]

Acknowledged.

+  dict->Remove("log", nullptr);
+}
+
 }  // namespace
 
 WebRTCInternals::PendingUpdate::PendingUpdate(
@@ -123,6 +129,9 @@ void WebRTCInternals::OnAddPeerConnection(int render_process_id,
                                           const string& constraints) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
+  // TODO(tommi): Consider changing this design so that webrtc-internals has
+  // minimal impact if chrome://webrtc-internals isn't open.
+
   std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
   dict->SetInteger("rid", render_process_id);
   dict->SetInteger("pid", static_cast<int>(pid));
@@ -148,28 +157,19 @@ void WebRTCInternals::OnAddPeerConnection(int render_process_id,
 
 void WebRTCInternals::OnRemovePeerConnection(ProcessId pid, int lid) {

[Henrik Grunell, 2017/03/03 12:40:09]

Great that you cleaned up the awkward code in this function!

[tommi (sloooow) - chröme, 2017/03/03 14:15:14]

:D

   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  for (size_t i = 0; i < peer_connection_data_.GetSize(); ++i) {
-    base::DictionaryValue* dict = NULL;
-    peer_connection_data_.GetDictionary(i, &dict);
-
-    int this_pid = 0;
-    int this_lid = 0;
-    dict->GetInteger("pid", &this_pid);
-    dict->GetInteger("lid", &this_lid);
-
-    if (this_pid != static_cast<int>(pid) || this_lid != lid)
-      continue;
 
+  size_t i;

[Henrik Grunell, 2017/03/03 12:40:09]

Nit: perhaps a more descriptive variable name?

[tommi (sloooow) - chröme, 2017/03/03 14:15:14]

Done... although I only changed it to 'index'.  |i| is what was being used
before and typically stands for index anyway.

+  base::DictionaryValue* dict = FindRecord(pid, lid, &i);
+  if (dict) {
     MaybeClosePeerConnection(dict);
     peer_connection_data_.Remove(i, NULL);
+  }
 
-    if (observers_.might_have_observers()) {
-      std::unique_ptr<base::DictionaryValue> id(new base::DictionaryValue());
-      id->SetInteger("pid", static_cast<int>(pid));
-      id->SetInteger("lid", lid);
-      SendUpdate("removePeerConnection", std::move(id));
-    }
-    break;
+  if (observers_.might_have_observers()) {
+    std::unique_ptr<base::DictionaryValue> id(new base::DictionaryValue());
+    id->SetInteger("pid", static_cast<int>(pid));
+    id->SetInteger("lid", lid);
+    SendUpdate("removePeerConnection", std::move(id));
   }
 }
 
@@ -177,48 +177,32 @@ void WebRTCInternals::OnUpdatePeerConnection(
     ProcessId pid, int lid, const string& type, const string& value) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  for (size_t i = 0; i < peer_connection_data_.GetSize(); ++i) {
-    base::DictionaryValue* record = NULL;
-    peer_connection_data_.GetDictionary(i, &record);
+  base::DictionaryValue* record = FindRecord(pid, lid);
+  if (record && type == "stop")
+    MaybeClosePeerConnection(record);
 
-    int this_pid = 0, this_lid = 0;
-    record->GetInteger("pid", &this_pid);
-    record->GetInteger("lid", &this_lid);
+  // Don't update entries if there aren't any observers.
+  if (!observers_.might_have_observers())
+    return;
 
-    if (this_pid != static_cast<int>(pid) || this_lid != lid)
-      continue;
+  std::unique_ptr<base::DictionaryValue> log_entry(new base::DictionaryValue());
 
-    if (type == "stop") {
-      MaybeClosePeerConnection(record);
-    }
+  double epoch_time = base::Time::Now().ToJsTime();
+  string time = base::DoubleToString(epoch_time);
+  log_entry->SetString("time", time);
+  log_entry->SetString("type", type);
+  log_entry->SetString("value", value);
 
-    // Append the update to the end of the log.
-    base::ListValue* log = EnsureLogList(record);
-    if (!log)
-      return;
-
-    std::unique_ptr<base::DictionaryValue> log_entry(
-        new base::DictionaryValue());
-
-    double epoch_time = base::Time::Now().ToJsTime();
-    string time = base::DoubleToString(epoch_time);
-    log_entry->SetString("time", time);
-    log_entry->SetString("type", type);
-    log_entry->SetString("value", value);
-
-    if (observers_.might_have_observers()) {
-      std::unique_ptr<base::DictionaryValue> update(
-          new base::DictionaryValue());
-      update->SetInteger("pid", static_cast<int>(pid));
-      update->SetInteger("lid", lid);
-      update->MergeDictionary(log_entry.get());
-
-      SendUpdate("updatePeerConnection", std::move(update));
-    }
+  std::unique_ptr<base::DictionaryValue> update(new base::DictionaryValue());
+  update->SetInteger("pid", static_cast<int>(pid));
+  update->SetInteger("lid", lid);
+  update->MergeDictionary(log_entry.get());
 
-    log->Append(std::move(log_entry));
+  SendUpdate("updatePeerConnection", std::move(update));

[Henrik Grunell, 2017/03/03 12:40:09]

In the previous code, this would not be done if no record is found, in the new
code it is. Is that intentional? (Maybe comment on why?)

[tommi (sloooow) - chröme, 2017/03/03 14:15:14]

Not intentional. Changed the behavior back now.

 
-    return;
+  if (record) {
+    // Append the update to the end of the log.
+    EnsureLogList(record)->Append(std::move(log_entry));
   }
 }
 
@@ -274,14 +258,17 @@ void WebRTCInternals::AddObserver(WebRTCInternalsUIObserver* observer) {
 void WebRTCInternals::RemoveObserver(WebRTCInternalsUIObserver* observer) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   observers_.RemoveObserver(observer);
+  if (observers_.might_have_observers())
+    return;
 
   // Disables event log and audio debug recordings if enabled and the last
   // webrtc-internals page is going away.
-  if (!observers_.might_have_observers()) {
-    if (audio_debug_recordings_)
-      DisableAudioDebugRecordings();
-    DisableEventLogRecordings();
-  }
+  DisableAudioDebugRecordings();
+  DisableEventLogRecordings();
+
+  // TODO(tommi): Consider removing all the peer_connection_data_.
+  for (auto& dictionary : peer_connection_data_)
+    FreeLogList(dictionary.get());

[Henrik Grunell, 2017/03/03 12:40:09]

I don't know the design of WebRTC internals, so to make sure: we don't lose any
data that we might want to display later when opening the
chrome://webrtc-internals page when doing this?

[tommi (sloooow) - chröme, 2017/03/03 14:15:14]

We do loose data but the thinking is that when chrome://webrtc-internals is
closed, that's a signal that the data isn't needed anymore.

[Henrik Grunell, 2017/03/06 11:49:24]

OK, if that's the intention that's fine then.

 }
 
 void WebRTCInternals::UpdateObserver(WebRTCInternalsUIObserver* observer) {
@@ -320,6 +307,9 @@ void WebRTCInternals::EnableAudioDebugRecordings(
 void WebRTCInternals::DisableAudioDebugRecordings() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 #if BUILDFLAG(ENABLE_WEBRTC)
+  if (!audio_debug_recordings_)
+    return;
+
   audio_debug_recordings_ = false;
 
   // Tear down the dialog since the user has unchecked the audio debug
@@ -577,4 +567,26 @@ void WebRTCInternals::ProcessPendingUpdates() {
   }
 }
 
+base::DictionaryValue* WebRTCInternals::FindRecord(
+    ProcessId pid,
+    int lid,
+    size_t* index /*= nullptr*/) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  base::DictionaryValue* record = nullptr;
+  for (size_t i = 0; i < peer_connection_data_.GetSize(); ++i) {
+    peer_connection_data_.GetDictionary(i, &record);
+
+    int this_pid = 0, this_lid = 0;
+    record->GetInteger("pid", &this_pid);
+    record->GetInteger("lid", &this_lid);
+
+    if (this_pid == static_cast<int>(pid) && this_lid == lid) {
+      if (index)
+        *index = i;
+      return record;
+    }
+  }
+  return nullptr;
+}
 }  // namespace content