Removing un-necessary conversion from std::string to base::string16.

chrome/browser/metrics/plugin_metrics_provider.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 "chrome/browser/metrics/plugin_metrics_provider.h"
 
 #include <string>
 
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
@@ skipping 206 matching lines @@
 
     base::DictionaryValue* plugin_dict =
         static_cast<base::DictionaryValue*>(*value_iter);
     std::string plugin_name;
     plugin_dict->GetString(prefs::kStabilityPluginName, &plugin_name);
     if (plugin_name.empty()) {
       NOTREACHED();
       continue;
     }
 
-    // TODO(viettrungluu): remove conversions
-    base::string16 name16 = base::UTF8ToUTF16(plugin_name);
-    if (child_process_stats_buffer_.find(name16) ==
+    if (child_process_stats_buffer_.find(plugin_name) ==
         child_process_stats_buffer_.end()) {
       continue;
     }
 
-    ChildProcessStats stats = child_process_stats_buffer_[name16];
+    ChildProcessStats stats = child_process_stats_buffer_[plugin_name];
     if (stats.process_launches) {
       int launches = 0;
       plugin_dict->GetInteger(prefs::kStabilityPluginLaunches, &launches);
       launches += stats.process_launches;
       plugin_dict->SetInteger(prefs::kStabilityPluginLaunches, launches);
     }
     if (stats.process_crashes) {
       int crashes = 0;
       plugin_dict->GetInteger(prefs::kStabilityPluginCrashes, &crashes);
       crashes += stats.process_crashes;
       plugin_dict->SetInteger(prefs::kStabilityPluginCrashes, crashes);
     }
     if (stats.instances) {
       int instances = 0;
       plugin_dict->GetInteger(prefs::kStabilityPluginInstances, &instances);
       instances += stats.instances;
       plugin_dict->SetInteger(prefs::kStabilityPluginInstances, instances);
     }
     if (stats.loading_errors) {
       int loading_errors = 0;
       plugin_dict->GetInteger(prefs::kStabilityPluginLoadingErrors,
                               &loading_errors);
       loading_errors += stats.loading_errors;
       plugin_dict->SetInteger(prefs::kStabilityPluginLoadingErrors,
                               loading_errors);
     }
 
-    child_process_stats_buffer_.erase(name16);
+    child_process_stats_buffer_.erase(plugin_name);
   }
 
   // Now go through and add dictionaries for plugins that didn't already have
   // reports in Local State.
-  for (std::map<base::string16, ChildProcessStats>::iterator cache_iter =
+  for (std::map<std::string, ChildProcessStats>::iterator cache_iter =

[Ilya Sherman, 2015/10/16 22:09:33]

nit: Please update this type to be "auto"

[Deepak, 2015/10/18 05:59:41]

Done.

            child_process_stats_buffer_.begin();
        cache_iter != child_process_stats_buffer_.end(); ++cache_iter) {
     ChildProcessStats stats = cache_iter->second;
 
     // Insert only plugins information into the plugins list.
     if (!IsPluginProcess(stats.process_type))
       continue;
 
-    // TODO(viettrungluu): remove conversion
-    std::string plugin_name = base::UTF16ToUTF8(cache_iter->first);
-
     base::DictionaryValue* plugin_dict = new base::DictionaryValue;
 
-    plugin_dict->SetString(prefs::kStabilityPluginName, plugin_name);
+    plugin_dict->SetString(prefs::kStabilityPluginName, cache_iter->first);
     plugin_dict->SetInteger(prefs::kStabilityPluginLaunches,
                             stats.process_launches);
     plugin_dict->SetInteger(prefs::kStabilityPluginCrashes,
                             stats.process_crashes);
     plugin_dict->SetInteger(prefs::kStabilityPluginInstances,
                             stats.instances);
     plugin_dict->SetInteger(prefs::kStabilityPluginLoadingErrors,
                             stats.loading_errors);
     plugins->Append(plugin_dict);
   }
   child_process_stats_buffer_.clear();
 }
 
 void PluginMetricsProvider::LogPluginLoadingError(
     const base::FilePath& plugin_path) {
   content::WebPluginInfo plugin;
   bool success =
       content::PluginService::GetInstance()->GetPluginInfoByPath(plugin_path,
                                                                  &plugin);
   DCHECK(success);
-  ChildProcessStats& stats = child_process_stats_buffer_[plugin.name];
+  ChildProcessStats& stats =
+      child_process_stats_buffer_[base::UTF16ToUTF8(plugin.name)];
   // Initialize the type if this entry is new.
   if (stats.process_type == content::PROCESS_TYPE_UNKNOWN) {
     // The plugin process might not actually be of type PLUGIN (which means
     // NPAPI), but we only care that it is *a* plugin process.
     stats.process_type = content::PROCESS_TYPE_PLUGIN;
   } else {
     DCHECK(IsPluginProcess(stats.process_type));
   }
   stats.loading_errors++;
   RecordCurrentStateWithDelay(kRecordStateDelayMs);
@@ skipping 17 matching lines @@
 }
 
 void PluginMetricsProvider::OnGotPlugins(
     const base::Closure& done_callback,
     const std::vector<content::WebPluginInfo>& plugins) {
   plugins_ = plugins;
   done_callback.Run();
 }
 
 PluginMetricsProvider::ChildProcessStats&
 PluginMetricsProvider::GetChildProcessStats(

[Ilya Sherman, 2015/10/16 22:09:33]

This method is fairly frequently called, so I'm not sure that it helps to trade
string conversions in a different place for string conversions here.  Best would
be to simply not have string conversions.

[Deepak, 2015/10/18 05:59:41]

I understand, It is best if we can avoid all string conversions.
But as kStabilityPluginName i.e plugin name should be stored in the dictionary
as std::string.
and plugin name in ChildProcessStats and in WebPluginInfo is base:string16.
So we have to do string conversion.

PluginMetricsProvider::RecordCurrentState() is also called frequently and here
we are doing conversion for every plugin in the map in 2 "for loops". that we
tried to avoid.
But again I completely agree with you,that it is trade off.
Please correct me if I misunderstood something.

[Ilya Sherman, 2015/10/20 06:24:32]

What makes you say that PluginMetricsProvider::RecordCurrentState() is
frequently called?

Regardless, I think the point of this TODO was to avoid all string conversions,
not just to trade some for others.  If I had to guess, I'd guess that the idea
was to unify the representation to consistently be either UTF-8 or UTF-16.

     const content::ChildProcessData& data) {
-  const base::string16& child_name = data.name;
+  const std::string& child_name = base::UTF16ToUTF8(data.name);
   if (!ContainsKey(child_process_stats_buffer_, child_name)) {
     child_process_stats_buffer_[child_name] =
         ChildProcessStats(data.process_type);
   }
   return child_process_stats_buffer_[child_name];
 }
 
 void PluginMetricsProvider::BrowserChildProcessHostConnected(
     const content::ChildProcessData& data) {
   GetChildProcessStats(data).process_launches++;
@@ skipping 36 matching lines @@
 }
 
 bool PluginMetricsProvider::RecordCurrentStateIfPending() {
   if (!weak_ptr_factory_.HasWeakPtrs())
     return false;
 
   weak_ptr_factory_.InvalidateWeakPtrs();
   RecordCurrentState();
   return true;
 }