Add helpers for logging mojom objects.

Add output formatters for more mojom types.

Add output formatters for printing the contents of Mojo arrays,
maps, and structs.  These are very useful for writing debug logs.

Committed: https://chromium.googlesource.com/external/mojo/+/7a6f49a41fb7f7616397a497e072b09373617c79

[jeffbrown, 2015-12-10 22:17:05]

jeffbrown@google.com changed reviewers:
+ vardhan@google.com

[jeffbrown, 2015-12-10 22:19:18]

jeffbrown@google.com changed reviewers:
+ abarth@google.com

[vardhan, 2015-12-11 00:12:20]

https://codereview.chromium.org/1519673002/diff/1/mojo/common/BUILD.gn
File mojo/common/BUILD.gn (right):

https://codereview.chromium.org/1519673002/diff/1/mojo/common/BUILD.gn#newcode12
mojo/common/BUILD.gn:12: "logging.h",
After looking at https://github.com/domokit/mojo/issues/480 and talking to
Trung..  I think the point of jamesr@'s comment on the github issue is to be
able to construct debug messages in a language agnostic way (by connecting and
querying a service).  Otherwise, we'll have to start maintaining printing
functions for each language.

I think it's better to hold off on making these mojom printing functions part of
mojo proper, and have them else where (inside your mojo/services/.. consumers?),
just so people don't start using it everywhere.

Maybe jamesr@ or vtl@ can comment if they have better suggestions

https://codereview.chromium.org/1519673002/diff/1/mojo/common/logging.h
File mojo/common/logging.h (right):

https://codereview.chromium.org/1519673002/diff/1/mojo/common/logging.h#newcode8
mojo/common/logging.h:8: #include <ostream>
nit: blank line after this include

https://codereview.chromium.org/1519673002/diff/1/mojo/common/logging.h#newco...
mojo/common/logging.h:13: // Mojom Logging helpers.
Probably "mojom debug string" is more clarifying; "logging" is a little
ambiguous?

https://codereview.chromium.org/1519673002/diff/1/mojo/common/logging.h#newco...
mojo/common/logging.h:19: template <typename T, typename = typename T::Data_>
Hm, I was hoping you could use IsStructPtr<> in bindings_internal.h, but
realized its in the mojo::internal namespace, so nevermind :(

https://codereview.chromium.org/1519673002/diff/1/mojo/common/logging.h#newco...
mojo/common/logging.h:20: auto operator<<(std::ostream& os, const T* value) ->
decltype(os << *value) {
What do you think about defining:

template <MojomWrapperType, typename enable = void>
void ToDebugString(MojomWrapperType val, std::ostream& output);

and specializing this for any type you want to be able to generate debug
strings?

i think overloading operator<<() is a little confusing because you could
conceptually define multiple semantics when "writing" mojom types, like write a
debug string /or/ write the serialized byte format (e.g., protobuf has APIs for
serializing to an ostream). so it might be better to be a bit more explicit with
this kind of API?

[jeffbrown, 2015-12-12 01:30:59]

Description was changed from

==========
Add helpers for logging mojom objects.

This is just an initial implementation to get the ball rolling.
It would be nice to move this code deeper into the bindings layer and
teach the mojom compiler how to generate sensible output formatters later.

BUG=
==========

to

==========
Add output formatters for more mojom types.
    
    Add output formatters for printing the contents of Mojo arrays,
    maps, and structs.  These are very useful for writing debug logs.
==========

[jeffbrown, 2015-12-12 01:31:14]

Description was changed from

==========
Add output formatters for more mojom types.
    
    Add output formatters for printing the contents of Mojo arrays,
    maps, and structs.  These are very useful for writing debug logs.
==========

to

==========
Add output formatters for more mojom types.

Add output formatters for printing the contents of Mojo arrays,
maps, and structs.  These are very useful for writing debug logs.
==========

[jeffbrown, 2015-12-12 01:34:53]

jeffbrown@google.com changed reviewers:
+ jamesr@chromium.org

[jeffbrown, 2015-12-12 01:34:53]

Ok here we go.  I moved the output formatters where they really belong.

I expect these functions to be generally useful to anyone writing C++ apps.  We
should consider whether to add ToString methods for Java and Dart bindings
likewise.

Not everything can be a service.

[jeffbrown, 2015-12-12 01:35:15]

jeffbrown@google.com changed reviewers:
+ vtl@google.com

[jeffbrown, 2015-12-12 01:35:15]

Ok here we go.  I moved the output formatters where they really belong.

I expect these functions to be generally useful to anyone writing C++
apps.  We
should consider whether to add ToString methods for Java and Dart bindings
likewise.

Not everything can be a service.

[viettrungluu, 2015-12-12 03:22:07]

viettrungluu@chromium.org changed reviewers:
+ viettrungluu@chromium.org
- vtl@google.com

[viettrungluu, 2015-12-15 16:09:38]

Let me list some of my concerns:

1. This takes away an area of customization from users of the library. In
particular, if the user wants to use it for non-debug output, with their own
formatting, they'd have a hard time doing so.

2. Relatedly, as a minor point, phrases like "for debugging purposes" basically
have no force.

3. Now, perhaps it'd be better if all the "debug-only" operator<<'s were in a
separate header/library, so that users have to explicitly choose to use them.
However, this is really a recipe for ODR violations.

4. If we change, e.g., mojo::Array<T> to std::optional<std::vector<T>> (or
std::unique_ptr<std::vector<T>>, since std::optional doesn't quite exist yet),
then we won't be able to provide operator<< -- it'd require defining something
in the std namespace, and this is undefined behavior. (This is not one of the
allowed cases; see 17.6.4.2.1 of the C++11 standard.)

(Note that we can currently define them in the mojo namespace due to ADL. If
both arguments are in the std namespace, then this won't work.)

-----

I think that if we want debug logging helpers like this, it'd be better to have
an overloaded mojo::ToDebugString() (or maybe mojo::debug::ToString(), or
whatever), in its own header(s).

[jeffbrown, 2015-12-17 19:33:26]

Description was changed from

==========
Add output formatters for more mojom types.

Add output formatters for printing the contents of Mojo arrays,
maps, and structs.  These are very useful for writing debug logs.
==========

to

==========
Add output formatters for more mojom types.

Add output formatters for printing the contents of Mojo arrays,
maps, and structs.  These are very useful for writing debug logs.

Committed:
https://chromium.googlesource.com/external/mojo/+/7a6f49a41fb7f7616397a497e07...
==========

[jeffbrown, 2015-12-17 19:33:27]

Committed patchset #3 (id:40001) manually as
7a6f49a41fb7f7616397a497e072b09373617c79.

[jeffbrown, 2015-12-17 19:33:53]

Blocked on this so submitting TBR.  We can refactor in the new year.

[jeffbrown, 2015-12-17 19:34:48]

Some good points however I feel we can be a little more opinionated about the
behavior of our framework.

1. Due to ADL, clients of Mojo would have difficulty defining operator overloads
for Mojo types in their own namespace anyhow.  If their formatting needs are
especially particular then I can easily envision them wanting to define their
own stream type rather than use std::ostream and being beholden to the standard
library's own behavior.

2. I can remove this phrase.  Really I just wanted something human readable
which is the general intent of the standard library output formatters anyhow. 
These formatters are consistent with that aim.

3. I tried this approach and it created some challenges.  a) There isn't a clear
place for the declarations to go.  b) Nearly everyone who defines output
formatters ends up wanting to include it.  c) Output formatters are easier to
find next to their related types for API usability.

4. I think we can cross that bridge when we get to it.  For now the Array, Map,
and String types belong to Mojo so it makes sense for us to specify how they are
formatted.  If we change the representation then there will be lots of other
things to fix at the same time and we can revisit our options then.

5. Mojo::ToDebugString is pretty clunky.  Moreover I really don't want to be
generating intermediate strings all over the place.  Output formatters already
neatly solve this problem and are compatible with our logging infrastructure.

All things considered I would very much like to proceed with the current
approach and see whether it causes any trouble down the road.

The big argument for me is API usability.  I don't think we lose anything by
defining these functions.

Thanks for your consideration.