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

We can change the key value to std::string in child_process_stats_buffer so that
conversion is not required while accessing value from map.

BUG=544002
Committed: https://crrev.com/2f7efbfc902e719648fa3b0f58cb66c3d1fede58
Cr-Commit-Position: refs/heads/master@{#355492}

[Deepak, 2015-10-16 08:59:26]

deepak.m1@samsung.com changed reviewers:
+ ajith.v@chromium.org

[Deepak, 2015-10-16 08:59:27]

PTAL

[AKV, 2015-10-16 13:03:56]

peer review looks good to me!

[Deepak, 2015-10-16 13:05:44]

deepak.m1@samsung.com changed reviewers:
+ isherman@chromium.org

[Deepak, 2015-10-16 13:05:45]

PTAL

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

https://codereview.chromium.org/1407203002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/plugin_metrics_provider.cc (right):

https://codereview.chromium.org/1407203002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/plugin_metrics_provider.cc:265: for
(std::map<std::string, ChildProcessStats>::iterator cache_iter =
nit: Please update this type to be "auto"

https://codereview.chromium.org/1407203002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/plugin_metrics_provider.cc:336:
PluginMetricsProvider::GetChildProcessStats(
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]

@Ilya
PTAL

https://codereview.chromium.org/1407203002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/plugin_metrics_provider.cc (right):

https://codereview.chromium.org/1407203002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/plugin_metrics_provider.cc:265: for
(std::map<std::string, ChildProcessStats>::iterator cache_iter =
On 2015/10/16 22:09:33, Ilya Sherman wrote:
> nit: Please update this type to be "auto"

Done.

https://codereview.chromium.org/1407203002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/plugin_metrics_provider.cc:336:
PluginMetricsProvider::GetChildProcessStats(
On 2015/10/16 22:09:33, Ilya Sherman wrote:
> 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.

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]

https://codereview.chromium.org/1407203002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/plugin_metrics_provider.cc (right):

https://codereview.chromium.org/1407203002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/plugin_metrics_provider.cc:336:
PluginMetricsProvider::GetChildProcessStats(
On 2015/10/18 05:59:41, Deepak wrote:
> On 2015/10/16 22:09:33, Ilya Sherman wrote:
> > 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.
> 
> 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.

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.

[Deepak, 2015-10-20 09:39:39]

On 2015/10/20 06:24:32, Ilya Sherman wrote:
>
https://codereview.chromium.org/1407203002/diff/20001/chrome/browser/metrics/...
> File chrome/browser/metrics/plugin_metrics_provider.cc (right):
> 
>
https://codereview.chromium.org/1407203002/diff/20001/chrome/browser/metrics/...
> chrome/browser/metrics/plugin_metrics_provider.cc:336:
> PluginMetricsProvider::GetChildProcessStats(
> On 2015/10/18 05:59:41, Deepak wrote:
> > On 2015/10/16 22:09:33, Ilya Sherman wrote:
> > > 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.
> > 
> > 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.
> 
> What makes you say that PluginMetricsProvider::RecordCurrentState() is
> frequently called?

The number of times GetChildProcessStats() is getting called, same number of
times
PluginMetricsProvider::RecordCurrentState() will get called via
RecordCurrentStateWithDelay().

> 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.

I agree with your opinion.
As plugin_dict->GetString() gives us value based on the out parameter passed in
the call.
if we are passing std::string as out parameter then we get data in std::string
format.
Similarly when we pass base::string16 then we get data in the base::string16
format.

plugin_dict->SetString() will store the data in the UTF8 format only.

I am made changes for this.
PTAL

[Deepak, 2015-10-22 04:16:41]

On 2015/10/20 09:39:39, Deepak wrote:
> On 2015/10/20 06:24:32, Ilya Sherman wrote:
> >
>
https://codereview.chromium.org/1407203002/diff/20001/chrome/browser/metrics/...
> > File chrome/browser/metrics/plugin_metrics_provider.cc (right):
> > 
> >
>
https://codereview.chromium.org/1407203002/diff/20001/chrome/browser/metrics/...
> > chrome/browser/metrics/plugin_metrics_provider.cc:336:
> > PluginMetricsProvider::GetChildProcessStats(
> > On 2015/10/18 05:59:41, Deepak wrote:
> > > On 2015/10/16 22:09:33, Ilya Sherman wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > What makes you say that PluginMetricsProvider::RecordCurrentState() is
> > frequently called?
> 
> The number of times GetChildProcessStats() is getting called, same number of
> times
> PluginMetricsProvider::RecordCurrentState() will get called via
> RecordCurrentStateWithDelay().
> 
> > 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.
> 
> I agree with your opinion.
> As plugin_dict->GetString() gives us value based on the out parameter passed
in
> the call.
> if we are passing std::string as out parameter then we get data in std::string
> format.
> Similarly when we pass base::string16 then we get data in the base::string16
> format.
> 
> plugin_dict->SetString() will store the data in the UTF8 format only.
> 
> I am made changes for this.
> PTAL

ping :)

[Ilya Sherman, 2015-10-22 04:27:45]

LGTM, thanks.

[Ilya Sherman, 2015-10-22 04:27:51]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2015-10-22 04:28:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407203002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407203002/60001

[Deepak, 2015-10-22 04:28:49]

On 2015/10/22 04:27:45, Ilya Sherman wrote:
> LGTM, thanks.

Thankyou Ilya.

[commit-bot: I haz the power, 2015-10-22 05:08:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-22 05:09:21]

Patchset 4 (id:??) landed as
https://crrev.com/2f7efbfc902e719648fa3b0f58cb66c3d1fede58
Cr-Commit-Position: refs/heads/master@{#355492}