Sync/Valgrind: Add gmock printers for SyncChange, SyncData, SyncError.

Sync/Valgrind: Add gmock printers for SyncChange, SyncData, SyncError.

Fixes a valgrind error in AutofillProfileSyncableServiceTest.ActOnChange
which uses gmock to mock a method that uses these classes. Due to
alignment issues, SyncData.is_valid_ has 3 bytes of uninitalized
padding. The default gmock printer prints all of these bytes, which
causes the uninit access.

BUG=none
TEST=Green memory waterfall

R=akalin@chromium.org


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

[James Hawkins, 2011-09-21 23:01:10]

CC: oshima

[akalin, 2011-09-21 23:12:58]

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_ch...
File chrome/browser/sync/api/sync_change.cc (right):

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_ch...
chrome/browser/sync/api/sync_change.cc:68: "{" << sync_change.change_type() <<
use ChangeTypeToString

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_ch...
File chrome/browser/sync/api/sync_change.h (right):

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_ch...
chrome/browser/sync/api/sync_change.h:61: std::ostream& operator<<(std::ostream&
os, const SyncChange& sync_change);
This is against Google style.  I'd prefer to add 'const std::string ToString()
const' member functions to SyncChange etc. and also define PrintTo() for gtest
as described in http://code.google.com/p/googletest/wiki/AdvancedGuide .

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_da...
File chrome/browser/sync/api/sync_data.cc (right):

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_da...
chrome/browser/sync/api/sync_data.cc:115: ", " << sync_data.GetTag() <<
print out specifics also: use EntitySpecificsToValue from
sync/protocol/proto_value_conversions.h and base/json/json_writer.h

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_da...
chrome/browser/sync/api/sync_data.cc:116: ", " << sync_data.GetTitle() <<
print out IsLocal(), too.  Also note that GetTag() DCHECKs for IsLocal().  If
IsLocal() returns false, print out GetRemoteId()

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_er...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_er...
chrome/browser/sync/api/sync_error.cc:99: ", " << sync_error.type() <<
print out location to: use location->ToString()

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_er...
chrome/browser/sync/api/sync_error.cc:99: ", " << sync_error.type() <<
use ModelTypeToString()

[James Hawkins, 2011-09-22 01:15:25]

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_ch...
File chrome/browser/sync/api/sync_change.cc (right):

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_ch...
chrome/browser/sync/api/sync_change.cc:68: "{" << sync_change.change_type() <<
On 2011/09/21 23:12:58, akalin wrote:
> use ChangeTypeToString

Done.

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_ch...
File chrome/browser/sync/api/sync_change.h (right):

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_ch...
chrome/browser/sync/api/sync_change.h:61: std::ostream& operator<<(std::ostream&
os, const SyncChange& sync_change);
On 2011/09/21 23:12:58, akalin wrote:
> This is against Google style.  I'd prefer to add 'const std::string ToString()
> const' member functions to SyncChange etc. and also define PrintTo() for gtest
> as described in http://code.google.com/p/googletest/wiki/AdvancedGuide .

Done.

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_da...
File chrome/browser/sync/api/sync_data.cc (right):

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_da...
chrome/browser/sync/api/sync_data.cc:115: ", " << sync_data.GetTag() <<
On 2011/09/21 23:12:58, akalin wrote:
> print out specifics also: use EntitySpecificsToValue from
> sync/protocol/proto_value_conversions.h and base/json/json_writer.h

Done.

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_da...
chrome/browser/sync/api/sync_data.cc:116: ", " << sync_data.GetTitle() <<
On 2011/09/21 23:12:58, akalin wrote:
> print out IsLocal(), too.  Also note that GetTag() DCHECKs for IsLocal().  If
> IsLocal() returns false, print out GetRemoteId()

Done.

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_er...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_er...
chrome/browser/sync/api/sync_error.cc:99: ", " << sync_error.type() <<
On 2011/09/21 23:12:58, akalin wrote:
> print out location to: use location->ToString()

Done.

http://codereview.chromium.org/7978044/diff/1/chrome/browser/sync/api/sync_er...
chrome/browser/sync/api/sync_error.cc:99: ", " << sync_error.type() <<
On 2011/09/21 23:12:58, akalin wrote:
> use ModelTypeToString()

Done.

[akalin, 2011-09-22 01:24:56]

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_change.cc (right):

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_change.cc:7: #include <iostream>
iostream -> ostream

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_data.cc (right):

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_data.cc:7: #include <iostream>
iostream -> ostream

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_data.cc:115: int64 id = IsLocal() ? 0 :
GetRemoteId();
prefer something like:

if (IsValid()) {
  if (IsLocal()) {
    // return string with type, tag, title, specifics
  } else {
    // return string with type, specifics, and id (title will hit the dcheck)
  }
} else {
  return "<Invalid SyncData>";
}

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:7: #include <iostream>
iostream -> ostream (former introduces a static intializer, iirc)

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:92: std::stringstream ss(out,
std::stringstream::in);
You don't really need a stringstream, do you?  Just build up the returned string
by concatenating

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:94: "{" << location_->ToString() <<
type() and message() CHECK IsSet().  Suggest:

if (IsSet()) {
  return "{" + location_->ToString() + ... + "}";
} else {
  return "<unset SyncError>";
}

[James Hawkins, 2011-09-22 02:50:29]

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_change.cc (right):

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_change.cc:7: #include <iostream>
On 2011/09/22 01:24:56, akalin wrote:
> iostream -> ostream

Done.

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_data.cc (right):

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_data.cc:7: #include <iostream>
On 2011/09/22 01:24:56, akalin wrote:
> iostream -> ostream

Done.

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_data.cc:115: int64 id = IsLocal() ? 0 :
GetRemoteId();
On 2011/09/22 01:24:56, akalin wrote:
> prefer something like:
> 
> if (IsValid()) {
>   if (IsLocal()) {
>     // return string with type, tag, title, specifics
>   } else {
>     // return string with type, specifics, and id (title will hit the dcheck)
>   }
> } else {
>   return "<Invalid SyncData>";
> }

Done.

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:7: #include <iostream>
On 2011/09/22 01:24:56, akalin wrote:
> iostream -> ostream (former introduces a static intializer, iirc)

Done.

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:92: std::stringstream ss(out,
std::stringstream::in);
On 2011/09/22 01:24:56, akalin wrote:
> You don't really need a stringstream, do you?  Just build up the returned
string
> by concatenating

I don't print IsSet() anymore, so I changed it back to string concat.

http://codereview.chromium.org/7978044/diff/1008/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:94: "{" << location_->ToString() <<
On 2011/09/22 01:24:56, akalin wrote:
> type() and message() CHECK IsSet().  Suggest:
> 
> if (IsSet()) {
>   return "{" + location_->ToString() + ... + "}";
> } else {
>   return "<unset SyncError>";
> }

Done.

[akalin, 2011-09-22 02:59:44]

Few more.  Almost there!

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_change.cc (right):

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_change.cc:66: return "{" +
ChangeTypeToString(change_type_) +
Prefer dictionary-style

return "{ changeType: " + ... + ", syncData: " + ... " }"

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_data.cc (right):

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_data.cc:126: std::string tag = IsLocal() ? GetTag()
: "";
std::string tag = GetTag();

Or just inline it to the below

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_data.cc:127: return "{" + type + ", " + tag + ", "
+ GetTitle() + ", " + specifics + "}";
return "{ isLocal: true, type: " + type + ", tag: " + ...

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_data.cc:130: std::string id =
UTF16ToUTF8(ui::FormatBytes(GetRemoteId()));
eh?  Just do base::Int64ToString(GetRemoteId()) (#include
base/string_number_conversions.h)

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_data.cc:131: return "{" + type + ", " + specifics +
", " + id + "}";
return "{ isLocal: false, type: " + ... + ", specifics: " + .. + "id: " + ... "
}"

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:91: return "{" + location_->ToString() +
", " + ", " +
return "{ location: " + ... + ", type: " + ... ", message: " + ... " }"

(also make sure you remove the doubled ", " string)

[akalin, 2011-09-22 20:26:01]

Ping!  I have some changes that could use ToString().

On 2011/09/22 02:59:44, akalin wrote:
> Few more.  Almost there!
> 
>
http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
> File chrome/browser/sync/api/sync_change.cc (right):
> 
>
http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
> chrome/browser/sync/api/sync_change.cc:66: return "{" +
> ChangeTypeToString(change_type_) +
> Prefer dictionary-style
> 
> return "{ changeType: " + ... + ", syncData: " + ... " }"
> 
>
http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
> File chrome/browser/sync/api/sync_data.cc (right):
> 
>
http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
> chrome/browser/sync/api/sync_data.cc:126: std::string tag = IsLocal() ?
GetTag()
> : "";
> std::string tag = GetTag();
> 
> Or just inline it to the below
> 
>
http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
> chrome/browser/sync/api/sync_data.cc:127: return "{" + type + ", " + tag + ",
"
> + GetTitle() + ", " + specifics + "}";
> return "{ isLocal: true, type: " + type + ", tag: " + ...
> 
>
http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
> chrome/browser/sync/api/sync_data.cc:130: std::string id =
> UTF16ToUTF8(ui::FormatBytes(GetRemoteId()));
> eh?  Just do base::Int64ToString(GetRemoteId()) (#include
> base/string_number_conversions.h)
> 
>
http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
> chrome/browser/sync/api/sync_data.cc:131: return "{" + type + ", " + specifics
+
> ", " + id + "}";
> return "{ isLocal: false, type: " + ... + ", specifics: " + .. + "id: " + ...
"
> }"
> 
>
http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
> File chrome/browser/sync/api/sync_error.cc (right):
> 
>
http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
> chrome/browser/sync/api/sync_error.cc:91: return "{" + location_->ToString() +
> ", " + ", " +
> return "{ location: " + ... + ", type: " + ... ", message: " + ... " }"
> 
> (also make sure you remove the doubled ", " string)

[James Hawkins, 2011-09-22 21:01:58]

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_change.cc (right):

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_change.cc:66: return "{" +
ChangeTypeToString(change_type_) +
On 2011/09/22 02:59:44, akalin wrote:
> Prefer dictionary-style
> 
> return "{ changeType: " + ... + ", syncData: " + ... " }"

Done.

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_data.cc (right):

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_data.cc:126: std::string tag = IsLocal() ? GetTag()
: "";
On 2011/09/22 02:59:44, akalin wrote:
> std::string tag = GetTag();
> 
> Or just inline it to the below

Done.

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_data.cc:127: return "{" + type + ", " + tag + ", "
+ GetTitle() + ", " + specifics + "}";
On 2011/09/22 02:59:44, akalin wrote:
> return "{ isLocal: true, type: " + type + ", tag: " + ...

Done.

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_data.cc:130: std::string id =
UTF16ToUTF8(ui::FormatBytes(GetRemoteId()));
On 2011/09/22 02:59:44, akalin wrote:
> eh?  Just do base::Int64ToString(GetRemoteId()) (#include
> base/string_number_conversions.h)

Done.

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_data.cc:131: return "{" + type + ", " + specifics +
", " + id + "}";
On 2011/09/22 02:59:44, akalin wrote:
> return "{ isLocal: false, type: " + ... + ", specifics: " + .. + "id: " + ...
"
> }"

Done.

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7978044/diff/6002/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:91: return "{" + location_->ToString() +
", " + ", " +
On 2011/09/22 02:59:44, akalin wrote:
> return "{ location: " + ... + ", type: " + ... ", message: " + ... " }"
> 
> (also make sure you remove the doubled ", " string)

Done.

[akalin, 2011-09-22 21:12:42]

LGTM after comments below

Also looks like you need to sync to head

http://codereview.chromium.org/7978044/diff/12001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_data.cc (right):

http://codereview.chromium.org/7978044/diff/12001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.cc:17: #include
"ui/base/text/bytes_formatting.h"
no need for this include

http://codereview.chromium.org/7978044/diff/12001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_data.cc:132: ", ID: " + id + "}";
prefer 'id' (Javascript/JSON-style casing)

[commit-bot: I haz the power, 2011-09-22 22:35:40]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/jhawkins@chromium.org/7978044/15001

[commit-bot: I haz the power, 2011-09-23 00:34:37]

Change committed as 102407

[oshima, 2011-09-24 03:08:45]

On 2011/09/23 00:34:37, I haz the power (commit-bot) wrote:
> Change committed as 102407

James, which bug/memory issue this was supposed to fix? I'll try to remove
suppression. thanks.