Make NameValuePairsParser support values that contain the 'equal' separator.

Make NameValuePairsParser support values that contain the 'equal' separator.

BUG=chromium-os:29642
TEST=unit_tests pass. ALEX devices and others report their HWID correctly to the UMA service.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133429

[petkov, 2012-04-19 13:47:01]

http://codereview.chromium.org/10127009/diff/1/chrome/browser/chromeos/system...
File chrome/browser/chromeos/system/statistics_provider.cc (right):

http://codereview.chromium.org/10127009/diff/1/chrome/browser/chromeos/system...
chrome/browser/chromeos/system/statistics_provider.cc:111: 
It might be a good idea to add another warning here if on_statistics_loaded_ is
not signaled yet. Somebody pasted some logs where we get two instances in a row
of the warning above which means that we lookup the map too early at least once.

Alternatively (or additionally), we should probably log a warning in
MetricsService::InitTaskGetHardwareClass if hardware_class_ is empty.

[1404:1404:5640558:ERROR:ownership_service.cc(80)] Blocking on UI thread in
OwnershipService::GetStatus
[1404:1433:5692610:WARNING:statistics_provider.cc(99)] Waiting to load
statistics. Requested statistic: hardware_class
[1404:1404:6021123:WARNING:statistics_provider.cc(99)] Waiting to load
statistics. Requested statistic: hardware_class
[1404:1433:8705821:WARNING:name_value_pairs_parser.cc(123)] Unable to read
statistics file: /tmp/machine-info
[1404:1404:8706558:ERROR:browser_policy_connector.cc(439)] Failed to get machine
serial number.
[1404:1433:8841407:ERROR:nss_util.cc(482)] Error initializing NSS with a
persistent database (sql:/etc/fake_root_ca/nssdb): NSS error code: -8174

[Mattias Nissler (ping if slow), 2012-04-19 13:51:39]

Looks reasonable to me, but I don't know this code, so I defer to somebody who
feels more attached to this.

http://codereview.chromium.org/10127009/diff/1/chrome/browser/chromeos/system...
File chrome/browser/chromeos/system/name_value_pairs_parser.cc (right):

http://codereview.chromium.org/10127009/diff/1/chrome/browser/chromeos/system...
chrome/browser/chromeos/system/name_value_pairs_parser.cc:21: const char
kTrimChars[] = "\" \t\r";
Are you sure this change is save? I.e. did you make sure the existing tools
don't use this and the Chrome-side code doesn't rely on these being present?

http://codereview.chromium.org/10127009/diff/1/chrome/browser/chromeos/system...
File chrome/browser/chromeos/system/name_value_pairs_parser.h (right):

http://codereview.chromium.org/10127009/diff/1/chrome/browser/chromeos/system...
chrome/browser/chromeos/system/name_value_pairs_parser.h:31: // The program name
|argv[0]| should be an absolute path. The function
There's been a recent discussion on whether or not to put bars around variables,
and I seem to remember that it was deemed optional. I personally like them, but
converting existing code written by others over to this style might be a bit
aggressive.

[Joao da Silva, 2012-04-19 16:01:30]

Adding @dpolukhin to review the changes to name_value_pairs_parser.

PTAL.

http://codereview.chromium.org/10127009/diff/1/chrome/browser/chromeos/system...
File chrome/browser/chromeos/system/name_value_pairs_parser.cc (right):

http://codereview.chromium.org/10127009/diff/1/chrome/browser/chromeos/system...
chrome/browser/chromeos/system/name_value_pairs_parser.cc:21: const char
kTrimChars[] = "\" \t\r";
On 2012/04/19 13:51:39, Mattias Nissler wrote:
> Are you sure this change is save? I.e. did you make sure the existing tools
> don't use this and the Chrome-side code doesn't rely on these being present?

Better leave it out for now :-) Everything seemed to work but testing wasn't
extensive.

http://codereview.chromium.org/10127009/diff/1/chrome/browser/chromeos/system...
File chrome/browser/chromeos/system/name_value_pairs_parser.h (right):

http://codereview.chromium.org/10127009/diff/1/chrome/browser/chromeos/system...
chrome/browser/chromeos/system/name_value_pairs_parser.h:31: // The program name
|argv[0]| should be an absolute path. The function
On 2012/04/19 13:51:39, Mattias Nissler wrote:
> There's been a recent discussion on whether or not to put bars around
variables,
> and I seem to remember that it was deemed optional. I personally like them,
but
> converting existing code written by others over to this style might be a bit
> aggressive.

I was under the impression that was the preferred style; reverted to the
original.

http://codereview.chromium.org/10127009/diff/1/chrome/browser/chromeos/system...
File chrome/browser/chromeos/system/statistics_provider.cc (right):

http://codereview.chromium.org/10127009/diff/1/chrome/browser/chromeos/system...
chrome/browser/chromeos/system/statistics_provider.cc:111: 
On 2012/04/19 13:47:01, petkov wrote:
> It might be a good idea to add another warning here if on_statistics_loaded_
is
> not signaled yet. Somebody pasted some logs where we get two instances in a
row
> of the warning above which means that we lookup the map too early at least
once.

Done.

> 
> Alternatively (or additionally), we should probably log a warning in
> MetricsService::InitTaskGetHardwareClass if hardware_class_ is empty.

Also done.

> 
> [1404:1404:5640558:ERROR:ownership_service.cc(80)] Blocking on UI thread in
> OwnershipService::GetStatus
> [1404:1433:5692610:WARNING:statistics_provider.cc(99)] Waiting to load
> statistics. Requested statistic: hardware_class
> [1404:1404:6021123:WARNING:statistics_provider.cc(99)] Waiting to load
> statistics. Requested statistic: hardware_class
> [1404:1433:8705821:WARNING:name_value_pairs_parser.cc(123)] Unable to read
> statistics file: /tmp/machine-info
> [1404:1404:8706558:ERROR:browser_policy_connector.cc(439)] Failed to get
machine
> serial number.
> [1404:1433:8841407:ERROR:nss_util.cc(482)] Error initializing NSS with a
> persistent database (sql:/etc/fake_root_ca/nssdb): NSS error code: -8174

[Dmitry Polukhin, 2012-04-23 08:34:48]

LGTM with nit

http://codereview.chromium.org/10127009/diff/7001/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/name_value_pairs_parser.cc (right):

http://codereview.chromium.org/10127009/diff/7001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/name_value_pairs_parser.cc:51: }
Should we report warning if given key already exists and we discard new value?

[Joao da Silva, 2012-04-23 09:13:17]

Thanks for reviewing!

http://codereview.chromium.org/10127009/diff/7001/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/name_value_pairs_parser.cc (right):

http://codereview.chromium.org/10127009/diff/7001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/name_value_pairs_parser.cc:51: }
On 2012/04/23 08:34:48, Dmitry Polukhin wrote:
> Should we report warning if given key already exists and we discard new value?

Yes, done. This is not expected to happen, but we really want to learn about it
if it does :-)

[petkov, 2012-04-23 09:34:06]

LGTM

http://codereview.chromium.org/10127009/diff/10002/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/name_value_pairs_parser.cc (right):

http://codereview.chromium.org/10127009/diff/10002/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/name_value_pairs_parser.cc:48: if
(map_->find(key) == map_->end()) {
you could use ContainsKey from base.

http://codereview.chromium.org/10127009/diff/10002/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/name_value_pairs_parser.cc:78: if (eq_pos !=
std::string::npos) {
We don't handle (i.e., ignore without setting is_valid to false) blank lines
here but that's probably OK.

http://codereview.chromium.org/10127009/diff/10002/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/10127009/diff/10002/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_service.cc:767: if (hardware_class.empty())
You could use LOG_IF instead.

[Joao da Silva, 2012-04-23 09:50:06]

http://codereview.chromium.org/10127009/diff/10002/chrome/browser/chromeos/sy...
File chrome/browser/chromeos/system/name_value_pairs_parser.cc (right):

http://codereview.chromium.org/10127009/diff/10002/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/name_value_pairs_parser.cc:48: if
(map_->find(key) == map_->end()) {
On 2012/04/23 09:34:06, petkov wrote:
> you could use ContainsKey from base.

Done.

http://codereview.chromium.org/10127009/diff/10002/chrome/browser/chromeos/sy...
chrome/browser/chromeos/system/name_value_pairs_parser.cc:78: if (eq_pos !=
std::string::npos) {
On 2012/04/23 09:34:06, petkov wrote:
> We don't handle (i.e., ignore without setting is_valid to false) blank lines
> here but that's probably OK.

The StringTokenizer takes care of that; it doesn't report empty "tokens". For
example, StringTokenizer("a = 1\n\nb = 2\n", "\n") only returns "a = 1" and "b =
2".

http://codereview.chromium.org/10127009/diff/10002/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/10127009/diff/10002/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_service.cc:767: if (hardware_class.empty())
On 2012/04/23 09:34:06, petkov wrote:
> You could use LOG_IF instead.

Another CL (http://codereview.chromium.org/10078017/) makes other changes to
metrics_service.cc such that this is now unnecessary. I've removed these lines.

[commit-bot: I haz the power, 2012-04-23 11:16:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10127009/22001

[commit-bot: I haz the power, 2012-04-23 12:50:03]

Change committed as 133429