Mojo Log service and a thread-safe client library.

Mojo Log service and a thread-safe client library.
The service impl itself is pretty thin and does what we currently already do:  print messages to stderr.
The log client provides a MojoLogger (you can hook this up to mojo::Environment and have MOJO_LOG() go directly to the Log service), and is thread-safe.

Other small changes to acommodate this CL:
 - mojo::Environment now has SetDefaultLogger() for replacing the logger.
 - Move mojo::internal::MessageBuilder out of internal namespace.

R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/bb3649854a7d7a5ec4c0c42055108b4d174cb151

[vardhan, 2015-11-16 23:57:24]

vardhan@google.com changed reviewers:
+ jamesr@chromium.org, viettrungluu@chromium.org

[vardhan, 2015-11-16 23:57:25]

PTAL.

Please let me know if you'd like me to break up this CL if you think it's too
long;  I'm assuming it's not too hard to read this :P

[jamesr, 2015-11-17 00:34:17]

https://codereview.chromium.org/1447273002/diff/20001/mojo/services/log/cpp/l...
File mojo/services/log/cpp/lib/log_client.cc (right):

https://codereview.chromium.org/1447273002/diff/20001/mojo/services/log/cpp/l...
mojo/services/log/cpp/lib/log_client.cc:32: InterfacePtrInfo<mojo::log::Log>
g_log_interface;
this will introduce a static destructor, which we don't want (in particular
it'll try to close the mojo handle during unload). it will cause races on
shutdown

[viettrungluu, 2015-11-17 22:33:09]

On 2015/11/16 23:57:25, vardhan wrote:
> PTAL.
> 
> Please let me know if you'd like me to break up this CL if you think it's too
> long;  I'm assuming it's not too hard to read this :P

Could you split this into two or three changes?
- One with the bindings changes.
- One with the logging stuff.
- Maybe one with the Environment logger changes (but you can fold this one into
the logging change).

[viettrungluu, 2015-11-17 22:33:33]

gah

https://codereview.chromium.org/1447273002/diff/40001/mojo/public/cpp/environ...
File mojo/public/cpp/environment/lib/environment.cc (right):

https://codereview.chromium.org/1447273002/diff/40001/mojo/public/cpp/environ...
mojo/public/cpp/environment/lib/environment.cc:26: 
nit: Adding this blank line seems gratuitous.

https://codereview.chromium.org/1447273002/diff/40001/mojo/public/cpp/environ...
mojo/public/cpp/environment/lib/environment.cc:68: assert(logger);
Maybe setting it to null should mean using &internal::kDefaultLogger?

[viettrungluu, 2015-11-20 23:21:51]

(I haven't fully reviewed the log client code yet.)

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
File mojo/services/log/cpp/lib/log_client.cc (right):

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
mojo/services/log/cpp/lib/log_client.cc:43: // We avoid the use of C++ bindings
to do interface calls in order to be
This comment belongs inside the function, not above it.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
mojo/services/log/cpp/lib/log_client.cc:45: // method of the Log interface does
not have a return type, we can easily do
it's not a "method"

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
mojo/services/log/cpp/lib/log_client.cc:98: // TODO(vardhan): What other cases
are there?
You should do this.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
File mojo/services/log/cpp/log_client.h (right):

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
mojo/services/log/cpp/log_client.h:14: // This is the client library that
constructs a MojoLogger that talks to the
*a* client library, not *the* client library

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
mojo/services/log/cpp/log_client.h:32: //  
Environment::SetDefaultLogger(LogClient::GetLogger());
This example code seems rather bad to me.

In particular, it's not obvious that the LogClient outlives the Environment.

Now that I think of it, that static Environment::SetDefaultLogger seems like a
bad idea.

It should be up to the particular implementation of |Environment| to decide how
the default logger should be set.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
File mojo/services/log/interfaces/entry.mojom (right):

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/entry.mojom:9: // invalid and should be taken to be
equivalent to |kLogLevelFatal|.
Then they're not invalid, are they?

Just say that they're equivalent to |kLogLevelFatal|.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/entry.mojom:10: 
nit: no blank line here

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/entry.mojom:17: // Describes a log message and its
origin in source code. It is used by the
The origin in source code is entirely incidental; that's just one type of
metadata. (We may add more metadata as desired.)

Note that source_file is optional.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/entry.mojom:22: // Log level: one of
|kLogLevel...|.
Not necessarily, as indicated by the comment above |kLogLevel...|.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/entry.mojom:27: uint32 source_line = 0;
Should comment that:
- This is a 1-based line number.
- 0 means invalid/ignored.
- The line number will be ignored if source_file is not present.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/entry.mojom:29: string? message;
|message| should come before source_file and source_line, being a more basic
thing.

(We'll nearly always want a messsage, even if source_file/source_line aren't
present or aren't applicable.)

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
File mojo/services/log/interfaces/log.mojom (right):

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/log.mojom:9: // A log interface provides a way to
do log source-code-level message entries.
I'd just say:

"An interface for logging, e.g., to the system log service."

(The source of the log messages can be anything, so saying anything about
"source code" is deceptive.)

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/log.mojom:12: // TODO(vardhan):  How do we
"globally silence" log entries issued by many
What do you mean "silence"?

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/log.mojom:13: // consumers of this interface? 
Should |Log| provide controls for log levels?
The answer to your second question is "no". In particular, what's the point?

If the client doesn't want the log service to do anything with a message, then
it can simply not send it.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/log.mojom:14: // or should clients register a
"LogClientController" with this interface, and
The answer to your third question is "maybe".

But the only utility of it would be to reduce the number of log messages
actually sent. The log service is free to filter/ignore/display log messages as
it desires, regardless of what the client thinks.

https://codereview.chromium.org/1447273002/diff/60001/services/log/BUILD.gn
File services/log/BUILD.gn (right):

https://codereview.chromium.org/1447273002/diff/60001/services/log/BUILD.gn#n...
services/log/BUILD.gn:1: # Copyright 2015 The Chromium Authors. All rights
reserved.
This should have its own tests, independent from the log client library's tests.

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.cc
File services/log/log_impl.cc (right):

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.c...
services/log/log_impl.cc:10: #include
"mojo/public/cpp/bindings/interface_request.h"
Probably don't need this include (already included by log_impl.h).

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.c...
services/log/log_impl.cc:17: namespace {
blank line after

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.c...
services/log/log_impl.cc:37: }  // namespace
blank line before

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.c...
services/log/log_impl.cc:40: : remote_url_(remote_url), binding_(this,
request.Pass()) {}
nit: should probably use std::move instead of .Pass() now (don't forget to
include <utility>)

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.c...
services/log/log_impl.cc:47: MOJO_DCHECK(connection);
Probably need to include some header for MOJO_DCHECK, etc.

(Note that since this is a service impl, it's actually OK to use //base, in
particular plain DCHECK, etc., from base/logging.h.)

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.c...
services/log/log_impl.cc:66: std::string rv = "<";
This would all be more readable if you just used StringPrintf (from
//base/strings/string_printf.h).

[vardhan, 2015-12-02 00:06:14]

PTAL

https://codereview.chromium.org/1447273002/diff/20001/mojo/services/log/cpp/l...
File mojo/services/log/cpp/lib/log_client.cc (right):

https://codereview.chromium.org/1447273002/diff/20001/mojo/services/log/cpp/l...
mojo/services/log/cpp/lib/log_client.cc:32: InterfacePtrInfo<mojo::log::Log>
g_log_interface;
On 2015/11/17 00:34:17, jamesr wrote:
> this will introduce a static destructor, which we don't want (in particular
> it'll try to close the mojo handle during unload). it will cause races on
> shutdown

crap.  I will make this a pointer, and have the LogClient destructor clean it
up.

https://codereview.chromium.org/1447273002/diff/40001/mojo/public/cpp/environ...
File mojo/public/cpp/environment/lib/environment.cc (right):

https://codereview.chromium.org/1447273002/diff/40001/mojo/public/cpp/environ...
mojo/public/cpp/environment/lib/environment.cc:68: assert(logger);
On 2015/11/17 22:33:33, viettrungluu wrote:
> Maybe setting it to null should mean using &internal::kDefaultLogger?

sounds right (Done.)

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
File mojo/services/log/cpp/lib/log_client.cc (right):

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
mojo/services/log/cpp/lib/log_client.cc:43: // We avoid the use of C++ bindings
to do interface calls in order to be
On 2015/11/20 23:21:51, viettrungluu wrote:
> This comment belongs inside the function, not above it.

Done.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
mojo/services/log/cpp/lib/log_client.cc:45: // method of the Log interface does
not have a return type, we can easily do
On 2015/11/20 23:21:51, viettrungluu wrote:
> it's not a "method"

Done.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
mojo/services/log/cpp/lib/log_client.cc:98: // TODO(vardhan): What other cases
are there?
On 2015/11/20 23:21:51, viettrungluu wrote:
> You should do this.

Done.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
File mojo/services/log/cpp/log_client.h (right):

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
mojo/services/log/cpp/log_client.h:14: // This is the client library that
constructs a MojoLogger that talks to the
On 2015/11/20 23:21:51, viettrungluu wrote:
> *a* client library, not *the* client library

Done.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/cpp/l...
mojo/services/log/cpp/log_client.h:32: //  
Environment::SetDefaultLogger(LogClient::GetLogger());
On 2015/11/20 23:21:51, viettrungluu wrote:
> This example code seems rather bad to me.
> 
> In particular, it's not obvious that the LogClient outlives the Environment.
> 
> Now that I think of it, that static Environment::SetDefaultLogger seems like a
> bad idea.
> 
> It should be up to the particular implementation of |Environment| to decide
how
> the default logger should be set.

I updated the comments to use ApplicationRunner instead of referring to
Environment's logger methods.

(we also discussed offline that we should move Environment's
Get/SetDefaultLogger methods out of Environment, as public global functions in
the mojo namespace.)

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
File mojo/services/log/interfaces/entry.mojom (right):

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/entry.mojom:9: // invalid and should be taken to be
equivalent to |kLogLevelFatal|.
On 2015/11/20 23:21:51, viettrungluu wrote:
> Then they're not invalid, are they?
> 
> Just say that they're equivalent to |kLogLevelFatal|.

Done.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/entry.mojom:10: 
On 2015/11/20 23:21:51, viettrungluu wrote:
> nit: no blank line here

Done.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/entry.mojom:17: // Describes a log message and its
origin in source code. It is used by the
On 2015/11/20 23:21:51, viettrungluu wrote:
> The origin in source code is entirely incidental; that's just one type of
> metadata. (We may add more metadata as desired.)
> 
> Note that source_file is optional.

I'll remove this bit then.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/entry.mojom:22: // Log level: one of
|kLogLevel...|.
On 2015/11/20 23:21:51, viettrungluu wrote:
> Not necessarily, as indicated by the comment above |kLogLevel...|.

Done.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/entry.mojom:27: uint32 source_line = 0;
On 2015/11/20 23:21:51, viettrungluu wrote:
> Should comment that:
> - This is a 1-based line number.
> - 0 means invalid/ignored.
> - The line number will be ignored if source_file is not present.

Done.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/entry.mojom:29: string? message;
On 2015/11/20 23:21:51, viettrungluu wrote:
> |message| should come before source_file and source_line, being a more basic
> thing.
> 
> (We'll nearly always want a messsage, even if source_file/source_line aren't
> present or aren't applicable.)

Done.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
File mojo/services/log/interfaces/log.mojom (right):

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/log.mojom:9: // A log interface provides a way to
do log source-code-level message entries.
On 2015/11/20 23:21:51, viettrungluu wrote:
> I'd just say:
> 
> "An interface for logging, e.g., to the system log service."
> 
> (The source of the log messages can be anything, so saying anything about
> "source code" is deceptive.)

Done.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/log.mojom:12: // TODO(vardhan):  How do we
"globally silence" log entries issued by many
On 2015/11/20 23:21:51, viettrungluu wrote:
> What do you mean "silence"?

(discussed offline)

This TODO is basically trying to address the need to control whatever is
displaying log messages to only display ERROR logs msgs, for example.  But this
control should not be related to this Log interface.

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/log.mojom:13: // consumers of this interface? 
Should |Log| provide controls for log levels?
On 2015/11/20 23:21:51, viettrungluu wrote:
> The answer to your second question is "no". In particular, what's the point?
> 
> If the client doesn't want the log service to do anything with a message, then
> it can simply not send it.

(discussed offline; see above)

https://codereview.chromium.org/1447273002/diff/60001/mojo/services/log/inter...
mojo/services/log/interfaces/log.mojom:14: // or should clients register a
"LogClientController" with this interface, and
On 2015/11/20 23:21:51, viettrungluu wrote:
> The answer to your third question is "maybe".
> 
> But the only utility of it would be to reduce the number of log messages
> actually sent. The log service is free to filter/ignore/display log messages
as
> it desires, regardless of what the client thinks.

(discussed offline; see above)

https://codereview.chromium.org/1447273002/diff/60001/services/log/BUILD.gn
File services/log/BUILD.gn (right):

https://codereview.chromium.org/1447273002/diff/60001/services/log/BUILD.gn#n...
services/log/BUILD.gn:1: # Copyright 2015 The Chromium Authors. All rights
reserved.
On 2015/11/20 23:21:51, viettrungluu wrote:
> This should have its own tests, independent from the log client library's
tests.

Done.

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.cc
File services/log/log_impl.cc (right):

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.c...
services/log/log_impl.cc:10: #include
"mojo/public/cpp/bindings/interface_request.h"
On 2015/11/20 23:21:51, viettrungluu wrote:
> Probably don't need this include (already included by log_impl.h).

Done.

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.c...
services/log/log_impl.cc:17: namespace {
On 2015/11/20 23:21:51, viettrungluu wrote:
> blank line after

Done.

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.c...
services/log/log_impl.cc:37: }  // namespace
On 2015/11/20 23:21:51, viettrungluu wrote:
> blank line before

Done.

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.c...
services/log/log_impl.cc:40: : remote_url_(remote_url), binding_(this,
request.Pass()) {}
On 2015/11/20 23:21:51, viettrungluu wrote:
> nit: should probably use std::move instead of .Pass() now (don't forget to
> include <utility>)

Done.

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.c...
services/log/log_impl.cc:47: MOJO_DCHECK(connection);
On 2015/11/20 23:21:51, viettrungluu wrote:
> Probably need to include some header for MOJO_DCHECK, etc.
> 
> (Note that since this is a service impl, it's actually OK to use //base, in
> particular plain DCHECK, etc., from base/logging.h.)

Done.  do you recommend using DCHECK or MOJO_DCHECK for things under //services
(unclear to me from your comment)?

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.c...
services/log/log_impl.cc:66: std::string rv = "<";
On 2015/11/20 23:21:51, viettrungluu wrote:
> This would all be more readable if you just used StringPrintf (from
> //base/strings/string_printf.h).

Done.

[viettrungluu, 2015-12-02 00:08:02]

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.cc
File services/log/log_impl.cc (right):

https://codereview.chromium.org/1447273002/diff/60001/services/log/log_impl.c...
services/log/log_impl.cc:47: MOJO_DCHECK(connection);
On 2015/12/02 00:06:14, vardhan wrote:
> On 2015/11/20 23:21:51, viettrungluu wrote:
> > Probably need to include some header for MOJO_DCHECK, etc.
> > 
> > (Note that since this is a service impl, it's actually OK to use //base, in
> > particular plain DCHECK, etc., from base/logging.h.)
> 
> Done.  do you recommend using DCHECK or MOJO_DCHECK for things under
//services
> (unclear to me from your comment)?

Probably, if you're using //base anyway, you should just use DCHECK, LOG, etc.

[viettrungluu, 2015-12-02 01:08:49]

https://codereview.chromium.org/1447273002/diff/120001/services/log/BUILD.gn
File services/log/BUILD.gn (right):

https://codereview.chromium.org/1447273002/diff/120001/services/log/BUILD.gn#...
services/log/BUILD.gn:27: "//mojo/public/cpp/application:standalone",
If you're using //base (which you are, via :log_impl), you probably shouldn't
use application:standalone.

[vardhan, 2015-12-02 01:47:58]

PTAL

https://codereview.chromium.org/1447273002/diff/120001/services/log/BUILD.gn
File services/log/BUILD.gn (right):

https://codereview.chromium.org/1447273002/diff/120001/services/log/BUILD.gn#...
services/log/BUILD.gn:27: "//mojo/public/cpp/application:standalone",
On 2015/12/02 01:08:49, viettrungluu wrote:
> If you're using //base (which you are, via :log_impl), you probably shouldn't
> use application:standalone.

Done.

[viettrungluu, 2015-12-03 19:13:25]

https://codereview.chromium.org/1447273002/diff/160001/mojo/services/log/cpp/...
File mojo/services/log/cpp/lib/log_client.cc (right):

https://codereview.chromium.org/1447273002/diff/160001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:36: std::atomic<MojoLogLevel>
g_min_log_level;
Hmmm. This may lead to a static initializer, probably. :-(

But I don't have a great solution for you.

You could, e.g., just declare memory for this, and do a placement new somewhere
appropriate (LogMessage's ctor).

There's no requirement that LogMessage's ctor be thread-safe.

So maybe the right thing to do is instead of having all g_log_interface and
g_min_log_level globals, just have a g_log_client global, move more of the
implementation of LogMessage() (etc.) into LogClient, and have LogMessage() just
forward to stuff in the class.

https://codereview.chromium.org/1447273002/diff/160001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:38: MojoLogger g_logclient_logger =
{&LogMessage, &GetMinimumLogLevel,
const

https://codereview.chromium.org/1447273002/diff/160001/mojo/services/log/cpp/...
File mojo/services/log/cpp/log_client.h (right):

https://codereview.chromium.org/1447273002/diff/160001/mojo/services/log/cpp/...
mojo/services/log/cpp/log_client.h:51: LogClient(LogPtr log_service, const
MojoLogger* fallback_logger);
Rather than exposing this class, maybe you should just expose
"InitializeLogging" and "ShutdownLogging" functions, and keep the class as an
internal detail?

[viettrungluu, 2015-12-11 23:56:47]

https://codereview.chromium.org/1447273002/diff/180001/services/log/log_impl.cc
File services/log/log_impl.cc (right):

https://codereview.chromium.org/1447273002/diff/180001/services/log/log_impl....
services/log/log_impl.cc:45: FILE* out_file)
Does this own out_file or not?

If so, you should probably use a ScopedFILE. (But then it'd be annoying to use
with stderr, etc. directly.)

If not, you should probably destroy your LogImpl before fclose()ing in your
test.

[vardhan, 2015-12-15 01:55:06]

ptal

https://codereview.chromium.org/1447273002/diff/160001/mojo/services/log/cpp/...
File mojo/services/log/cpp/lib/log_client.cc (right):

https://codereview.chromium.org/1447273002/diff/160001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:36: std::atomic<MojoLogLevel>
g_min_log_level;
On 2015/12/03 19:13:25, viettrungluu wrote:
> Hmmm. This may lead to a static initializer, probably. :-(
> 
> But I don't have a great solution for you.
> 
> You could, e.g., just declare memory for this, and do a placement new
somewhere
> appropriate (LogMessage's ctor).
> 
> There's no requirement that LogMessage's ctor be thread-safe.
> 
> So maybe the right thing to do is instead of having all g_log_interface and
> g_min_log_level globals, just have a g_log_client global, move more of the
> implementation of LogMessage() (etc.) into LogClient, and have LogMessage()
just
> forward to stuff in the class.

Done. I have a global |LogClient* g_log_client| that dynamically allocate in
InitializeLogger()

how do you propose "declaring memory and doing a placement new"?  I understand
it to mean declaring a |char g_bytes_for_logclient[sizeof(LogClient)]| that
backs some LogClient* pointer, and doing a placement new on that pointer which i
guess avoids the dynamic allocation, but.. seems weird?

https://codereview.chromium.org/1447273002/diff/160001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:38: MojoLogger g_logclient_logger =
{&LogMessage, &GetMinimumLogLevel,
On 2015/12/03 19:13:25, viettrungluu wrote:
> const

Done.

https://codereview.chromium.org/1447273002/diff/160001/mojo/services/log/cpp/...
File mojo/services/log/cpp/log_client.h (right):

https://codereview.chromium.org/1447273002/diff/160001/mojo/services/log/cpp/...
mojo/services/log/cpp/log_client.h:51: LogClient(LogPtr log_service, const
MojoLogger* fallback_logger);
On 2015/12/03 19:13:25, viettrungluu wrote:
> Rather than exposing this class, maybe you should just expose
> "InitializeLogging" and "ShutdownLogging" functions, and keep the class as an
> internal detail?

Done.

https://codereview.chromium.org/1447273002/diff/180001/services/log/log_impl.cc
File services/log/log_impl.cc (right):

https://codereview.chromium.org/1447273002/diff/180001/services/log/log_impl....
services/log/log_impl.cc:45: FILE* out_file)
On 2015/12/11 23:56:47, viettrungluu wrote:
> Does this own out_file or not?
> 
> If so, you should probably use a ScopedFILE. (But then it'd be annoying to use
> with stderr, etc. directly.)
> 
> If not, you should probably destroy your LogImpl before fclose()ing in your
> test.
Thanks!

I chose the latter (for the reason you mentioned).

I also don't explicit destroy LogImpl; instead, I reset the client-side LogPtr,
which /should/ invoke a connection error in the strong binding, which "owns" the
LogImpl.  I'm not exactly sure what the ownership model should be when a strong
binding is involved.. ?

[viettrungluu, 2015-12-15 18:33:08]

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
File mojo/services/log/cpp/BUILD.gn (right):

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/BUILD.gn:13: restrict_external_deps = false
Do you actually need this, if you using $mojo_sdk_root instead of ..?

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/BUILD.gn:15: public_configs = [
"../../public/build/config:mojo_services" ]
Should you write this using $mojo_sdk_root instead?

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/BUILD.gn:23: "../interfaces",
Should you write this using $mojo_sdk_root?

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/BUILD.gn:49:
"$mojo_sdk_root/mojo/services/log/interfaces",
Admittedly, it's weird to use $mojo_sdk_root for something that's not even in
the SDK proper. Hmmm.

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
File mojo/services/log/cpp/lib/log_client.cc (right):

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:53: std::atomic<MojoLogLevel>
min_loglevel_;
min_log_level_

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:55: };
MOJO_DISALLOW_COPY_AND_ASSIGN, please.

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:99: auto retval =
"result" instead of "retval", since "retval"/"rv"/etc. are usually used for what
*you* are going to return.

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:114: log_interface_.PassHandle();
I wonder if InterfacePtrInfo shouldn't have a reset() method....

More significantly, this isn't threadsafe.

Probably you should:
* Declare all of your members const.
* Not close it on error, which is a little sad, but oh well.

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
File services/log/log_impl_unittest.cc (right):

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
services/log/log_impl_unittest.cc:51: pipe(p);
You should, say, ASSERT_EQ(0, pipe(p));.

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
services/log/log_impl_unittest.cc:53: FILE* output_file = fdopen(p[1], "w");
Probably should use ScopedFILE (base/files/scoped_file.h).

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
services/log/log_impl_unittest.cc:59: LogImpl::Create(&app_connection,
std::move(log_request), output_file);
Why don't you just directly put GetProxy(&log) in here, instead of using a
temporary + std::move?

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
services/log/log_impl_unittest.cc:82: base::TimeDelta::FromMilliseconds(10));
Probably should use TestTimeouts::tiny_timeout(), from
base/test/test_timeouts.h.

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
services/log/log_impl_unittest.cc:87: const char* expected_string =
const char expected_string[] = ...

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
services/log/log_impl_unittest.cc:93: size_t nbytes = read(p[0], actual_string,
strlen(expected_string));
Theoretically, this should be wrapped in HANDLE_EINTR, but ... <shrug>.

Also, read() returns an ssize_t, not a size_t; you should assert that what it
returns is nonnegative.

Also, you should include <string.h> for strlen().

[vardhan, 2015-12-16 18:29:05]

PTAL

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
File mojo/services/log/cpp/BUILD.gn (right):

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/BUILD.gn:13: restrict_external_deps = false
On 2015/12/15 18:33:08, viettrungluu wrote:
> Do you actually need this, if you using $mojo_sdk_root instead of ..?
I based this build file off of files_impl build file, and made some assumptions
based off of it: 

I assumed that this /may/ not be part of a the mojo SDK proper (but dont know if
i can use anything other than a mojo_sdk_source_set() target) since it's not
part of mojo/public?  is $mojo_sdk_root assumed to be defined outside of the
SDK?

is mojo/services going to be part of the SDK proper?
if not, what can i use instead of mojo_sdk_source_set?
if so, should i (in addition to addressing your comments) also move the relative
deps=[] into fully-qualified mojo_sdk_deps?

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/BUILD.gn:15: public_configs = [
"../../public/build/config:mojo_services" ]
On 2015/12/15 18:33:08, viettrungluu wrote:
> Should you write this using $mojo_sdk_root instead?

(see above)

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/BUILD.gn:23: "../interfaces",
On 2015/12/15 18:33:08, viettrungluu wrote:
> Should you write this using $mojo_sdk_root?

(see above)

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/BUILD.gn:49:
"$mojo_sdk_root/mojo/services/log/interfaces",
On 2015/12/15 18:33:08, viettrungluu wrote:
> Admittedly, it's weird to use $mojo_sdk_root for something that's not even in
> the SDK proper. Hmmm.

yeah.. i'm a little confused if $mojo_sdk_root can be expected to be  defined
outside of the SDK proper (i.e, for use in build files under mojo/services)

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
File mojo/services/log/cpp/lib/log_client.cc (right):

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:53: std::atomic<MojoLogLevel>
min_loglevel_;
On 2015/12/15 18:33:08, viettrungluu wrote:
> min_log_level_

Done. (removed this all together)

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:55: };
On 2015/12/15 18:33:08, viettrungluu wrote:
> MOJO_DISALLOW_COPY_AND_ASSIGN, please.

Done.

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:99: auto retval =
On 2015/12/15 18:33:08, viettrungluu wrote:
> "result" instead of "retval", since "retval"/"rv"/etc. are usually used for
what
> *you* are going to return.

Done.

https://codereview.chromium.org/1447273002/diff/200001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:114: log_interface_.PassHandle();
On 2015/12/15 18:33:08, viettrungluu wrote:
> I wonder if InterfacePtrInfo shouldn't have a reset() method....
> 
> More significantly, this isn't threadsafe.
> 
> Probably you should:
> * Declare all of your members const.
> * Not close it on error, which is a little sad, but oh well.

Done.

(going to delegate set/get min log level to the fallback logger and get rid of
any state all together)

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
File services/log/log_impl_unittest.cc (right):

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
services/log/log_impl_unittest.cc:51: pipe(p);
On 2015/12/15 18:33:08, viettrungluu wrote:
> You should, say, ASSERT_EQ(0, pipe(p));.

Done.

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
services/log/log_impl_unittest.cc:53: FILE* output_file = fdopen(p[1], "w");
On 2015/12/15 18:33:08, viettrungluu wrote:
> Probably should use ScopedFILE (base/files/scoped_file.h).

I figured it wasn't necessary since i'm closing it manually below, but I guess
its better to be defensive

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
services/log/log_impl_unittest.cc:59: LogImpl::Create(&app_connection,
std::move(log_request), output_file);
On 2015/12/15 18:33:08, viettrungluu wrote:
> Why don't you just directly put GetProxy(&log) in here, instead of using a
> temporary + std::move?

Done.

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
services/log/log_impl_unittest.cc:82: base::TimeDelta::FromMilliseconds(10));
On 2015/12/15 18:33:08, viettrungluu wrote:
> Probably should use TestTimeouts::tiny_timeout(), from
> base/test/test_timeouts.h.

Done.

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
services/log/log_impl_unittest.cc:87: const char* expected_string =
On 2015/12/15 18:33:08, viettrungluu wrote:
> const char expected_string[] = ...

Done.

https://codereview.chromium.org/1447273002/diff/200001/services/log/log_impl_...
services/log/log_impl_unittest.cc:93: size_t nbytes = read(p[0], actual_string,
strlen(expected_string));
On 2015/12/15 18:33:08, viettrungluu wrote:
> Theoretically, this should be wrapped in HANDLE_EINTR, but ... <shrug>.
> 
> Also, read() returns an ssize_t, not a size_t; you should assert that what it
> returns is nonnegative.
> 
> Also, you should include <string.h> for strlen().

Done.  (also, im thinking the EXPECT_EQ() is sufficient and don't need the
additional non-negative assert)

[viettrungluu, 2015-12-17 18:34:40]

https://codereview.chromium.org/1447273002/diff/220001/mojo/public/cpp/enviro...
File mojo/public/cpp/environment/environment.h (right):

https://codereview.chromium.org/1447273002/diff/220001/mojo/public/cpp/enviro...
mojo/public/cpp/environment/environment.h:34: // TODO(vardhan):  Require that
this logger is thread-safe.
It occurs to me that ... I'm not *that* stupid.

In the definition of MojoLogger (//mojo/public/c/environment/logger.h), the
thread-safety requirements are precisely defined.

(Basically, it says that things are thread-safe, except for
SetMinimumLogLevel(), which must not be called concurrently with other stuff.)

So you can remove this TODO.

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
File mojo/services/log/cpp/lib/log_client.cc (right):

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:48: void SetMinimumLogLevel(MojoLogLevel
level) const;
I guess this wouldn't be const, according to the requirements I put on
MojoLogger.

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:52: const MojoLogger* fallback_logger_;
You could make this more const (|const MojoLogger* const|). <shrug>

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:54:
MOJO_DISALLOW_COPY_AND_ASSIGN(LogClient);
nit: include the appropriate header for this

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
File mojo/services/log/cpp/log_client.h (right):

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
mojo/services/log/cpp/log_client.h:42: // to the provided |log_service| fails,
and so must be non-null and thread-safe.
"and thread-safe" is redundant, I guess.

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
File mojo/services/log/cpp/tests/log_client_unittest.cc (right):

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
mojo/services/log/cpp/tests/log_client_unittest.cc:5: #include <atomic>
Apparently, you don't need any atomics, according to the specifications for
MojoLogger.

https://codereview.chromium.org/1447273002/diff/220001/services/log/log_impl_...
File services/log/log_impl_unittest.cc (right):

https://codereview.chromium.org/1447273002/diff/220001/services/log/log_impl_...
services/log/log_impl_unittest.cc:96:
EXPECT_EQ(static_cast<ssize_t>(strlen(expected_string)), nbytes);
This really should be |ASSERT_EQ|, since otherwise your |EXPECT_STREQ| below may
crash. <shrug>

[vardhan, 2015-12-17 23:35:42]

PTAL

https://codereview.chromium.org/1447273002/diff/220001/mojo/public/cpp/enviro...
File mojo/public/cpp/environment/environment.h (right):

https://codereview.chromium.org/1447273002/diff/220001/mojo/public/cpp/enviro...
mojo/public/cpp/environment/environment.h:34: // TODO(vardhan):  Require that
this logger is thread-safe.
On 2015/12/17 18:34:39, viettrungluu wrote:
> It occurs to me that ... I'm not *that* stupid.
> 
> In the definition of MojoLogger (//mojo/public/c/environment/logger.h), the
> thread-safety requirements are precisely defined.
> 
> (Basically, it says that things are thread-safe, except for
> SetMinimumLogLevel(), which must not be called concurrently with other stuff.)
> 
> So you can remove this TODO.

Done.

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
File mojo/services/log/cpp/lib/log_client.cc (right):

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:48: void SetMinimumLogLevel(MojoLogLevel
level) const;
On 2015/12/17 18:34:39, viettrungluu wrote:
> I guess this wouldn't be const, according to the requirements I put on
> MojoLogger.

Done.  (but technically, this method is still const since it is delegating its
calls to fallback logger still)

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:52: const MojoLogger* fallback_logger_;
On 2015/12/17 18:34:39, viettrungluu wrote:
> You could make this more const (|const MojoLogger* const|). <shrug>

Done.

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
mojo/services/log/cpp/lib/log_client.cc:54:
MOJO_DISALLOW_COPY_AND_ASSIGN(LogClient);
On 2015/12/17 18:34:39, viettrungluu wrote:
> nit: include the appropriate header for this

Done.

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
File mojo/services/log/cpp/log_client.h (right):

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
mojo/services/log/cpp/log_client.h:42: // to the provided |log_service| fails,
and so must be non-null and thread-safe.
On 2015/12/17 18:34:39, viettrungluu wrote:
> "and thread-safe" is redundant, I guess.

Done.

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
File mojo/services/log/cpp/tests/log_client_unittest.cc (right):

https://codereview.chromium.org/1447273002/diff/220001/mojo/services/log/cpp/...
mojo/services/log/cpp/tests/log_client_unittest.cc:5: #include <atomic>
On 2015/12/17 18:34:39, viettrungluu wrote:
> Apparently, you don't need any atomics, according to the specifications for
> MojoLogger.

Done.

https://codereview.chromium.org/1447273002/diff/220001/services/log/log_impl_...
File services/log/log_impl_unittest.cc (right):

https://codereview.chromium.org/1447273002/diff/220001/services/log/log_impl_...
services/log/log_impl_unittest.cc:96:
EXPECT_EQ(static_cast<ssize_t>(strlen(expected_string)), nbytes);
On 2015/12/17 18:34:39, viettrungluu wrote:
> This really should be |ASSERT_EQ|, since otherwise your |EXPECT_STREQ| below
may
> crash. <shrug>

Done.

[viettrungluu, 2015-12-18 00:32:56]

We have passed 100 comments (triple digits!), so LGTM, with one more thing.

https://codereview.chromium.org/1447273002/diff/240001/mojo/services/log/cpp/...
File mojo/services/log/cpp/tests/log_client_unittest.cc (right):

https://codereview.chromium.org/1447273002/diff/240001/mojo/services/log/cpp/...
mojo/services/log/cpp/tests/log_client_unittest.cc:50: MojoLogLevel
g_fallback_logger_level = MOJO_LOG_LEVEL_INFO;
You should set |g_fallback_logger_level| and |g_fallback_logger_invoked| at the
beginning of the test below, in case this test is repeated. (In that case, you
don't really *need* to initialize it here, I guess.)

Actually, come to think of it, I guess you don't even need these to be global,
since you're using lambdas, right?

https://codereview.chromium.org/1447273002/diff/240001/mojo/services/log/cpp/...
mojo/services/log/cpp/tests/log_client_unittest.cc:51: bool
g_fallback_logger_invoked = false;
nit: add a blank line below

[vardhan, 2015-12-18 03:56:29]

PTAL at comments (submitting later tomorrow otherwise)

https://codereview.chromium.org/1447273002/diff/240001/mojo/services/log/cpp/...
File mojo/services/log/cpp/tests/log_client_unittest.cc (right):

https://codereview.chromium.org/1447273002/diff/240001/mojo/services/log/cpp/...
mojo/services/log/cpp/tests/log_client_unittest.cc:50: MojoLogLevel
g_fallback_logger_level = MOJO_LOG_LEVEL_INFO;
On 2015/12/18 00:32:56, viettrungluu wrote:
> You should set |g_fallback_logger_level| and |g_fallback_logger_invoked| at
the
> beginning of the test below, in case this test is repeated. (In that case, you
> don't really *need* to initialize it here, I guess.)
Good point!  Done.

> Actually, come to think of it, I guess you don't even need these to be global,
> since you're using lambdas, right?
I need these to be global because I can't use these two variables as captures
(otherwise, the lambdas don't have function signatures compatible with
MojoLogger)

https://codereview.chromium.org/1447273002/diff/240001/mojo/services/log/cpp/...
mojo/services/log/cpp/tests/log_client_unittest.cc:51: bool
g_fallback_logger_invoked = false;
On 2015/12/18 00:32:56, viettrungluu wrote:
> nit: add a blank line below

Done.

[viettrungluu, 2015-12-18 17:35:55]

On 2015/12/18 03:56:29, vardhan wrote:
> PTAL at comments (submitting later tomorrow otherwise)
> 
>
https://codereview.chromium.org/1447273002/diff/240001/mojo/services/log/cpp/...
> File mojo/services/log/cpp/tests/log_client_unittest.cc (right):
> 
>
https://codereview.chromium.org/1447273002/diff/240001/mojo/services/log/cpp/...
> mojo/services/log/cpp/tests/log_client_unittest.cc:50: MojoLogLevel
> g_fallback_logger_level = MOJO_LOG_LEVEL_INFO;
> On 2015/12/18 00:32:56, viettrungluu wrote:
> > You should set |g_fallback_logger_level| and |g_fallback_logger_invoked| at
> the
> > beginning of the test below, in case this test is repeated. (In that case,
you
> > don't really *need* to initialize it here, I guess.)
> Good point!  Done.
> 
> > Actually, come to think of it, I guess you don't even need these to be
global,
> > since you're using lambdas, right?
> I need these to be global because I can't use these two variables as captures
> (otherwise, the lambdas don't have function signatures compatible with
> MojoLogger)

Oh, right. Well, the real reason is that otherwise the lambdas aren't just
function pointers. (If you have captures, then a lambda is an object.)

Actually, the real real reason is that lambdas are always objects, but when they
have no capture then they have an implicit conversion operator to a function
pointer (and presumably the call operator is static).

> 
>
https://codereview.chromium.org/1447273002/diff/240001/mojo/services/log/cpp/...
> mojo/services/log/cpp/tests/log_client_unittest.cc:51: bool
> g_fallback_logger_invoked = false;
> On 2015/12/18 00:32:56, viettrungluu wrote:
> > nit: add a blank line below
> 
> Done.

[vardhan, 2015-12-18 19:09:09]

Description was changed from

==========
Mojo Log service and a thread-safe client library.
The service impl itself is pretty thin and does what we currently already do: 
print messages to stderr.
The log client provides a MojoLogger (you can hook this up to mojo::Environment
and have MOJO_LOG() go directly to the Log service), and is thread-safe.

Other small changes to acommodate this CL:
 - mojo::Environment now has SetDefaultLogger() for replacing the logger.
 - Move mojo::internal::MessageBuilder out of internal namespace.
==========

to

==========
Mojo Log service and a thread-safe client library.
The service impl itself is pretty thin and does what we currently already do: 
print messages to stderr.
The log client provides a MojoLogger (you can hook this up to mojo::Environment
and have MOJO_LOG() go directly to the Log service), and is thread-safe.

Other small changes to acommodate this CL:
 - mojo::Environment now has SetDefaultLogger() for replacing the logger.
 - Move mojo::internal::MessageBuilder out of internal namespace.

R=viettrungluu@chromium.org

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

[vardhan, 2015-12-18 19:09:10]

Committed patchset #14 (id:260001) manually as
bb3649854a7d7a5ec4c0c42055108b4d174cb151 (presubmit successful).