Add ToString method for easy DLOG and debug support

Override << operator for easier debugging.


BUG=
TEST=


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

[Steve McKay, 2012-06-22 19:23:18]

Hi Ben. Can you take a look at this?

[benwells, 2012-06-22 19:47:48]

lgtm with nit

http://codereview.chromium.org/10559049/diff/1/chrome/common/custom_handlers/...
File chrome/common/custom_handlers/protocol_handler.cc (right):

http://codereview.chromium.org/10559049/diff/1/chrome/common/custom_handlers/...
chrome/common/custom_handlers/protocol_handler.cc:93: }
nit: chrome style is to put the << at the beginning of new lines.

[Peter Kasting, 2012-06-22 20:20:32]

http://codereview.chromium.org/10559049/diff/1/chrome/common/custom_handlers/...
File chrome/common/custom_handlers/protocol_handler.h (right):

http://codereview.chromium.org/10559049/diff/1/chrome/common/custom_handlers/...
chrome/common/custom_handlers/protocol_handler.h:71: std::ostream&
operator<<(std::ostream& os, const ProtocolHandler& handler);
Technically to use this you need to #include <ostream>.

This in turn pulls in the stream headers into anyone who #includes this.  That's
not a great thing (and bordering on a style guide violation, in that the style
guide tries to lock down streams fairly tightly).

The usual way we solve this is to instead provide a ToString() method or similar
that can return a string representation.  Then test code or other callers who
are already using streams can just do "<< obj.ToString()", and we don't taint
any more code than is necessary with streams.

[smckay, 2012-06-22 21:57:43]

PTAL.

http://codereview.chromium.org/10559049/diff/1/chrome/common/custom_handlers/...
File chrome/common/custom_handlers/protocol_handler.h (right):

http://codereview.chromium.org/10559049/diff/1/chrome/common/custom_handlers/...
chrome/common/custom_handlers/protocol_handler.h:71: std::ostream&
operator<<(std::ostream& os, const ProtocolHandler& handler);
On 2012/06/22 20:20:32, Peter Kasting wrote:
> Technically to use this you need to #include <ostream>.
> 
> This in turn pulls in the stream headers into anyone who #includes this. 
That's
> not a great thing (and bordering on a style guide violation, in that the style
> guide tries to lock down streams fairly tightly).
> 
> The usual way we solve this is to instead provide a ToString() method or
similar
> that can return a string representation.  Then test code or other callers who
> are already using streams can just do "<< obj.ToString()", and we don't taint
> any more code than is necessary with streams.

Done.

Q: Does the general distaste for streams then apply to things like
std::stringstream?

[Peter Kasting, 2012-06-22 22:10:06]

Q: Does the general distaste for streams then apply to things like
std::stringstream?
A: Yes.

http://codereview.chromium.org/10559049/diff/7001/chrome/common/custom_handle...
File chrome/common/custom_handlers/protocol_handler.cc (right):

http://codereview.chromium.org/10559049/diff/7001/chrome/common/custom_handle...
chrome/common/custom_handlers/protocol_handler.cc:74: return std::string("{ ") +
Nit: I'd probably write/wrap as:

  return "{ protocol=" + protocol_ + ", url=" + url_ + ", title=" + title_ +
      " }";

The more important bits here are, include the trailing underscores on your
variable names, and you can omit the std::string() at the beginning + not place
two spaces in a row if you combine the two opening string constants.  :)

http://codereview.chromium.org/10559049/diff/7001/chrome/common/custom_handle...
File chrome/common/custom_handlers/protocol_handler.h (right):

http://codereview.chromium.org/10559049/diff/7001/chrome/common/custom_handle...
chrome/common/custom_handlers/protocol_handler.h:57: std::string ToString()
const;
Nit: Consider a comment about why we have this.

Are your needs for this restricted to, say, debugging or unittests?  In those
cases we can further limit access here.

[Steve McKay, 2012-06-22 22:40:01]

PTAL. Now with COMPILES!

http://codereview.chromium.org/10559049/diff/7001/chrome/common/custom_handle...
File chrome/common/custom_handlers/protocol_handler.cc (right):

http://codereview.chromium.org/10559049/diff/7001/chrome/common/custom_handle...
chrome/common/custom_handlers/protocol_handler.cc:74: return std::string("{ ") +
On 2012/06/22 22:10:06, Peter Kasting wrote:
> Nit: I'd probably write/wrap as:
> 
>   return "{ protocol=" + protocol_ + ", url=" + url_ + ", title=" + title_ +
>       " }";
> 
> The more important bits here are, include the trailing underscores on your
> variable names

Sorry. Mailed before I compiled :(

Had to add some more code to make this compile (since not all fields are
strings). Doesn't really hold up now in the long "less wrapped" form. Happy to
rewrap differently if specify what you want.

http://codereview.chromium.org/10559049/diff/7001/chrome/common/custom_handle...
File chrome/common/custom_handlers/protocol_handler.h (right):

http://codereview.chromium.org/10559049/diff/7001/chrome/common/custom_handle...
chrome/common/custom_handlers/protocol_handler.h:57: std::string ToString()
const;
On 2012/06/22 22:10:06, Peter Kasting wrote:
> Nit: Consider a comment about why we have this.
> 
> Are your needs for this restricted to, say, debugging or unittests?  In those
> cases we can further limit access here.

Added a comment.

Basically I pulled this out of another CL as my new team mates complained that
my CLs were too big. The uses I have are in DLOG statements and debugging.

[Peter Kasting, 2012-06-22 22:45:24]

LGTM; if your only use of this is for DLOG, I suggest wrapping it in #if
!defined(NDEBUG).

[smckay, 2012-06-22 23:25:37]

On 2012/06/22 22:45:24, Peter Kasting wrote:
> LGTM; if your only use of this is for DLOG, I suggest wrapping it in #if
> !defined(NDEBUG).

Good idea. Done!

[Peter Kasting, 2012-06-22 23:45:48]

LGTM

http://codereview.chromium.org/10559049/diff/12004/chrome/common/custom_handl...
File chrome/common/custom_handlers/protocol_handler.cc (right):

http://codereview.chromium.org/10559049/diff/12004/chrome/common/custom_handl...
chrome/common/custom_handlers/protocol_handler.cc:8: #include
"base/stringprintf.h"
Nit: This include not needed

http://codereview.chromium.org/10559049/diff/12004/chrome/common/custom_handl...
chrome/common/custom_handlers/protocol_handler.cc:75: # if !defined(NDEBUG)
Nit: No space after # (2 places)

http://codereview.chromium.org/10559049/diff/12004/chrome/common/custom_handl...
File chrome/common/custom_handlers/protocol_handler.h (right):

http://codereview.chromium.org/10559049/diff/12004/chrome/common/custom_handl...
chrome/common/custom_handlers/protocol_handler.h:57: # if !defined(NDEBUG)
Nit: No space after # (2 places)

[Steve McKay, 2012-06-27 22:34:38]

Thanks. PTAL

http://codereview.chromium.org/10559049/diff/12004/chrome/common/custom_handl...
File chrome/common/custom_handlers/protocol_handler.cc (right):

http://codereview.chromium.org/10559049/diff/12004/chrome/common/custom_handl...
chrome/common/custom_handlers/protocol_handler.cc:8: #include
"base/stringprintf.h"
On 2012/06/22 23:45:48, Peter Kasting wrote:
> Nit: This include not needed

Done.

http://codereview.chromium.org/10559049/diff/12004/chrome/common/custom_handl...
chrome/common/custom_handlers/protocol_handler.cc:75: # if !defined(NDEBUG)
On 2012/06/22 23:45:48, Peter Kasting wrote:
> Nit: No space after # (2 places)

Done.

http://codereview.chromium.org/10559049/diff/12004/chrome/common/custom_handl...
File chrome/common/custom_handlers/protocol_handler.h (right):

http://codereview.chromium.org/10559049/diff/12004/chrome/common/custom_handl...
chrome/common/custom_handlers/protocol_handler.h:57: # if !defined(NDEBUG)
On 2012/06/22 23:45:48, Peter Kasting wrote:
> Nit: No space after # (2 places)

Done.

[Peter Kasting, 2012-06-27 22:40:15]

LGTM

[commit-bot: I haz the power, 2012-06-27 23:55:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/smckay@chromium.org/10559049/21001

[commit-bot: I haz the power, 2012-06-28 01:03:55]

Change committed as 144638