Pass error messages from native messaging to web-app.

Pass error messages from native messaging to web-app.

This replaces a single onDone(result) callback with a pair (onDone(), onError(message, location)). The error message and location are communicated via native messaging as additional parameters, which should ensure backwards-compatibility in both directions.

BUG=517176
Committed: https://crrev.com/a1826488343b1923906c2b8bbd194eef51cfda26
Cr-Commit-Position: refs/heads/master@{#344622}

[Jamie, 2015-08-05 22:27:28]

jamiewalch@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Jamie, 2015-08-05 22:27:28]

This is not complete yet (it needs implementing on Mac and Windows, and there
are some ancilliary binaries like start_host that don't compile as yet) but I
wanted some early feedback on whether or not this seems like a reasonable
approach before I go too far into the weeds.

[Sergey Ulanov, 2015-08-06 22:40:57]

If the goal is just to get the errors in the JS console then another possible
approach is to forward all LOG() messages from the NM process to the webapp,
similar to how we do in in the client plugin. We'll just to add hooks for LOG(),
similar to how it's implemented in 
ChromotingInstance::RegisterLogMessageHandler(). That would allow forwarding all
LOG messages, not just a single message per request. It would also be a simpler
solution as it would require any changes platform-specific code. WDYT?

https://codereview.chromium.org/1272833002/diff/40001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host.cc (right):

https://codereview.chromium.org/1272833002/diff/40001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.cc:230: std::ostringstream
error_message;
This code may be simpler with base::StringPrintf() or just appending strings
together.

https://codereview.chromium.org/1272833002/diff/40001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/host_daemon_facade.js (right):

https://codereview.chromium.org/1272833002/diff/40001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/host_daemon_facade.js:207:
reply.deferred.resolve(message);
should we call reject() in case there was an error?

[Jamie, 2015-08-06 23:06:17]

Regarding echoing the logs, that might be useful in addition to this, but I
don't think it's quite what we're looking for here. By structuring the code such
that it isn't possible to send a failure response without providing an error
message and a location, it means that the logs stay clean if all goes well. It
also ensures that the errors are logged with console.error, which is not the
case for our existing echoing solution (though presumably we could make that
work).

This approach also makes it relatively simple to add in explicit error codes in
the future so that common failure modes can be distinguished and reported to the
user, rather than being hidden in the log.

https://codereview.chromium.org/1272833002/diff/40001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host.cc (right):

https://codereview.chromium.org/1272833002/diff/40001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.cc:230: std::ostringstream
error_message;
On 2015/08/06 22:40:57, Sergey Ulanov wrote:
> This code may be simpler with base::StringPrintf() or just appending strings
> together.

I'm not a fan of printf, but the +-operator would be clearer (assuming it works
for all the types we need).

https://codereview.chromium.org/1272833002/diff/40001/remoting/webapp/crd/js/...
File remoting/webapp/crd/js/host_daemon_facade.js (right):

https://codereview.chromium.org/1272833002/diff/40001/remoting/webapp/crd/js/...
remoting/webapp/crd/js/host_daemon_facade.js:207:
reply.deferred.resolve(message);
On 2015/08/06 22:40:57, Sergey Ulanov wrote:
> should we call reject() in case there was an error?

I don't think so. The contract here is that the caller requests that a message
be sent and the Promise is fulfilled when the reply arrives, whether it be
success or failure. This code should ideally be generic and not attempt any
parsing of the message at all (TBH, I'm not convinced that this is the right
place to print the error, but the alternative is duplicating it at each
call-site, which seems worse).

[Sergey Ulanov, 2015-08-08 00:57:10]

I don't like separate callbacks for success and error. It adds more boilerplate
code and also doesn't allow to handle CANCELLED response.

https://codereview.chromium.org/1272833002/diff/60001/remoting/host/setup/dae...
File remoting/host/setup/daemon_controller.cc (right):

https://codereview.chromium.org/1272833002/diff/60001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller.cc:60:
&DaemonController::InvokeErrorCallbackAndScheduleNext, this, on_error);
Do we need separate on_done and on_error cases? I think this code would be
simpler if there was only one completion callback. If you are concerned about
passing error message when there is no error, then maybe add a class for the
result with an optional error field? I.e. make AsyncResult a class or struct
instead of enum.

https://codereview.chromium.org/1272833002/diff/60001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller.cc:176: if
(!caller_task_runner_->BelongsToCurrentThread()) {
Delegate interface definition says that the callback must be called on the
original thread, so I'm not sure this is necessary.

https://codereview.chromium.org/1272833002/diff/60001/remoting/host/setup/hos...
File remoting/host/setup/host_starter.cc (right):

https://codereview.chromium.org/1272833002/diff/60001/remoting/host/setup/hos...
remoting/host/setup/host_starter.cc:189: if
(!main_task_runner_->BelongsToCurrentThread()) {
I don't think we need this thread-check. DaemonController should be calling the
callback on the thread it is called.

https://codereview.chromium.org/1272833002/diff/60001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host.cc (left):

https://codereview.chromium.org/1272833002/diff/60001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.cc:499:
response->SetString("result", "CANCELLED");
Do we still send "CANCELLED" response? I think it was useful when user
explicitly cancels an operation, e.g. UAC prompt. In that case we don't want to
show the error in the webapp?

https://codereview.chromium.org/1272833002/diff/60001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host.cc (right):

https://codereview.chromium.org/1272833002/diff/60001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.cc:6: #include <sstream>
don't need this include.
nit: add an empty line here.

[Jamie, 2015-08-11 21:23:13]

I decided to take your previous advice and just send log messages to the
web-app. I still think there's value in sending errors in a more structured way,
but let's wait and see what sort of errors turn out to be the most common before
adding the necessary plumbing.

The implementation here should work on Mac and Linux. I think I'll need to add
additional plumbing to get these messages over the elevated channel to make it
work on Windows.

https://codereview.chromium.org/1272833002/diff/60001/remoting/host/setup/me2...
File remoting/host/setup/me2me_native_messaging_host.cc (left):

https://codereview.chromium.org/1272833002/diff/60001/remoting/host/setup/me2...
remoting/host/setup/me2me_native_messaging_host.cc:499:
response->SetString("result", "CANCELLED");
On 2015/08/08 00:57:09, Sergey Ulanov wrote:
> Do we still send "CANCELLED" response? I think it was useful when user
> explicitly cancels an operation, e.g. UAC prompt. In that case we don't want
to
> show the error in the webapp?

I've reverted this change, but FWIW I don't think it's currently sent (see
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/setu...)

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
File remoting/host/native_messaging/log_message_handler.h (right):

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.h:19: typedef
base::Callback<void (scoped_ptr<base::Value> message)> Delegate;
This is only ever passed a DictionaryValue, but giving it the same signature as
NativeMessagingChannel::SendMessage avoids a wrapper function.

[Jamie, 2015-08-14 18:25:11]

On 2015/08/11 21:23:13, Jamie wrote:
> I decided to take your previous advice and just send log messages to the
> web-app. I still think there's value in sending errors in a more structured
way,
> but let's wait and see what sort of errors turn out to be the most common
before
> adding the necessary plumbing.
> 
> The implementation here should work on Mac and Linux. I think I'll need to add
> additional plumbing to get these messages over the elevated channel to make it
> work on Windows.
> 
>
https://codereview.chromium.org/1272833002/diff/60001/remoting/host/setup/me2...
> File remoting/host/setup/me2me_native_messaging_host.cc (left):
> 
>
https://codereview.chromium.org/1272833002/diff/60001/remoting/host/setup/me2...
> remoting/host/setup/me2me_native_messaging_host.cc:499:
> response->SetString("result", "CANCELLED");
> On 2015/08/08 00:57:09, Sergey Ulanov wrote:
> > Do we still send "CANCELLED" response? I think it was useful when user
> > explicitly cancels an operation, e.g. UAC prompt. In that case we don't want
> to
> > show the error in the webapp?
> 
> I've reverted this change, but FWIW I don't think it's currently sent (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/setu...)
> 
>
https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
> File remoting/host/native_messaging/log_message_handler.h (right):
> 
>
https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
> remoting/host/native_messaging/log_message_handler.h:19: typedef
> base::Callback<void (scoped_ptr<base::Value> message)> Delegate;
> This is only ever passed a DictionaryValue, but giving it the same signature
as
> NativeMessagingChannel::SendMessage avoids a wrapper function.

Ping

[Sergey Ulanov, 2015-08-16 23:22:03]

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host.cc:118:
log_message_handler_.reset(
This should be disabled on ChromeOS. We don't want to log all messages from the
browser process.

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
File remoting/host/native_messaging/log_message_handler.cc (right):

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
nit: remove (c)

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:54: bool
LogMessageHandler::OnLogMessage(
add //static comment to make clear that this is a static method.

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:58: if
(g_log_message_handler.Get()) {
WeakPtr can be deferenced only on one thread, so you can't check it here. I
realize that there is a lock, but there is a DCHECK in WeakPtr implementations
for threads. I don't think you really need WeakPtr. You can store a raw pointer
- it's reset in the destructor anyway.

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:67: size_t message_start,
const std::string& str) {
nit: one parameter per line please

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:72: if (suppress_logging_)
{
I think this is necessary only on the caller thread, where suppress_logging_ is
set. I.e. you need caller_task_runner_->BelongsToCurrentThread() condition here.

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:78: // This alows the lock
to be released quickly.
One downside of this approach is that LOG_FATAL errors will always be lost.
That's not a big issue here because the messages will still show up in stderr
(unlike the client plugin). Still worth calling it out here in the comment.

Potentially this can be fixed. E.g. even just writing all messages synchronously
(with a lock) would be acceptable - that wouldn't hurt performance much because
there is a buffer for the pipe in the OS, but that would require changing how we
send all messages, not just for logging.

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:102: std::string message =
str.c_str() + message_start;
message = std.substr(message_start);

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
File remoting/host/native_messaging/log_message_handler.h (right):

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.h:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
nit: remove (c)

https://codereview.chromium.org/1272833002/diff/100001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/host_daemon_facade.js (right):

https://codereview.chromium.org/1272833002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/host_daemon_facade.js:194:
console.error('NativeMessaging: missing or non-numeric id');
old webapp versions will log this error with new host. Maybe add a field in the
"hello" message to the host  to indicate that logging is supported. We have
"supportedFeatures" field in the hello response.

https://codereview.chromium.org/1272833002/diff/100001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/native_message_host_log_message_handler.js (right):

https://codereview.chromium.org/1272833002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/native_message_host_log_message_handler.js:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
2015

[Jamie, 2015-08-17 17:32:24]

PTAL

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_native_messaging_host.cc (right):

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host.cc:118:
log_message_handler_.reset(
On 2015/08/16 23:22:02, Sergey Ulanov wrote:
> This should be disabled on ChromeOS. We don't want to log all messages from
the
> browser process.

Done.

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
File remoting/host/native_messaging/log_message_handler.cc (right):

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
On 2015/08/16 23:22:02, Sergey Ulanov wrote:
> nit: remove (c)

Done.

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:54: bool
LogMessageHandler::OnLogMessage(
On 2015/08/16 23:22:02, Sergey Ulanov wrote:
> add //static comment to make clear that this is a static method.

Done.

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:58: if
(g_log_message_handler.Get()) {
On 2015/08/16 23:22:02, Sergey Ulanov wrote:
> WeakPtr can be deferenced only on one thread, so you can't check it here. I
> realize that there is a lock, but there is a DCHECK in WeakPtr implementations
> for threads. I don't think you really need WeakPtr. You can store a raw
pointer
> - it's reset in the destructor anyway.

Doesn't that introduce a race if the LogMessageHandler is being destroyed on one
thread while another thread is logging? If OnLogMessage wins the race and grabs
the lock before the dtor, then by the time the message is processed on the other
thread, it will be invoking it on a dead object. This doesn't seem like a
particularly uncommon pattern; is there a standard solution?

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:67: size_t message_start,
const std::string& str) {
On 2015/08/16 23:22:02, Sergey Ulanov wrote:
> nit: one parameter per line please

Done.

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:72: if (suppress_logging_)
{
On 2015/08/16 23:22:02, Sergey Ulanov wrote:
> I think this is necessary only on the caller thread, where suppress_logging_
is
> set. I.e. you need caller_task_runner_->BelongsToCurrentThread() condition
here.

Done.

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:78: // This alows the lock
to be released quickly.
On 2015/08/16 23:22:02, Sergey Ulanov wrote:
> One downside of this approach is that LOG_FATAL errors will always be lost.
> That's not a big issue here because the messages will still show up in stderr
> (unlike the client plugin). Still worth calling it out here in the comment.
> 
> Potentially this can be fixed. E.g. even just writing all messages
synchronously
> (with a lock) would be acceptable - that wouldn't hurt performance much
because
> there is a buffer for the pipe in the OS, but that would require changing how
we
> send all messages, not just for logging.

Done.

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:102: std::string message =
str.c_str() + message_start;
On 2015/08/16 23:22:02, Sergey Ulanov wrote:
> message = std.substr(message_start);

Done.

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
File remoting/host/native_messaging/log_message_handler.h (right):

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.h:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
On 2015/08/16 23:22:02, Sergey Ulanov wrote:
> nit: remove (c)

Done.

https://codereview.chromium.org/1272833002/diff/100001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/host_daemon_facade.js (right):

https://codereview.chromium.org/1272833002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/host_daemon_facade.js:194:
console.error('NativeMessaging: missing or non-numeric id');
On 2015/08/16 23:22:02, Sergey Ulanov wrote:
> old webapp versions will log this error with new host. Maybe add a field in
the
> "hello" message to the host  to indicate that logging is supported. We have
> "supportedFeatures" field in the hello response.

We don't expect the version mismatch to last very long (maybe a week or so in
the case of a %age push) and the error message is benign. I agree it's not
ideal, but plumbing it via hello risks missing some messages and complicates the
mechanism more than I like.

This is only a mild preference on my part; LMK if you feel strongly about it.

https://codereview.chromium.org/1272833002/diff/100001/remoting/webapp/crd/js...
File remoting/webapp/crd/js/native_message_host_log_message_handler.js (right):

https://codereview.chromium.org/1272833002/diff/100001/remoting/webapp/crd/js...
remoting/webapp/crd/js/native_message_host_log_message_handler.js:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/08/16 23:22:02, Sergey Ulanov wrote:
> 2015

Done.

[Sergey Ulanov, 2015-08-17 21:01:16]

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
File remoting/host/native_messaging/log_message_handler.cc (right):

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:58: if
(g_log_message_handler.Get()) {
On 2015/08/17 17:32:23, Jamie wrote:
> On 2015/08/16 23:22:02, Sergey Ulanov wrote:
> > WeakPtr can be deferenced only on one thread, so you can't check it here. I
> > realize that there is a lock, but there is a DCHECK in WeakPtr
implementations
> > for threads. I don't think you really need WeakPtr. You can store a raw
> pointer
> > - it's reset in the destructor anyway.
> 
> Doesn't that introduce a race if the LogMessageHandler is being destroyed on
one
> thread while another thread is logging? If OnLogMessage wins the race and
grabs
> the lock before the dtor, then by the time the message is processed on the
other
> thread, it will be invoking it on a dead object. This doesn't seem like a
> particularly uncommon pattern; is there a standard solution?

You can still post the task using weak pointer. PostLogMessageToCorrectThread()
already calls GetWeakPtr() to post the task. That's safe because it doesn't
require dereferencing the pointer. You just can't call WeakPtr::Get() on other
threads.
g_log_message_handler is only used with g_lock held, so it doesn't need to be a
weak pointer.

[Jamie, 2015-08-17 21:23:35]

PTAL

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
File remoting/host/native_messaging/log_message_handler.cc (right):

https://codereview.chromium.org/1272833002/diff/100001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:58: if
(g_log_message_handler.Get()) {
On 2015/08/17 21:01:16, Sergey Ulanov wrote:
> On 2015/08/17 17:32:23, Jamie wrote:
> > On 2015/08/16 23:22:02, Sergey Ulanov wrote:
> > > WeakPtr can be deferenced only on one thread, so you can't check it here.
I
> > > realize that there is a lock, but there is a DCHECK in WeakPtr
> implementations
> > > for threads. I don't think you really need WeakPtr. You can store a raw
> > pointer
> > > - it's reset in the destructor anyway.
> > 
> > Doesn't that introduce a race if the LogMessageHandler is being destroyed on
> one
> > thread while another thread is logging? If OnLogMessage wins the race and
> grabs
> > the lock before the dtor, then by the time the message is processed on the
> other
> > thread, it will be invoking it on a dead object. This doesn't seem like a
> > particularly uncommon pattern; is there a standard solution?
> 
> You can still post the task using weak pointer.
PostLogMessageToCorrectThread()
> already calls GetWeakPtr() to post the task. That's safe because it doesn't
> require dereferencing the pointer. You just can't call WeakPtr::Get() on other
> threads.
> g_log_message_handler is only used with g_lock held, so it doesn't need to be
a
> weak pointer.

Done.

[Sergey Ulanov, 2015-08-17 23:06:25]

lgtm

https://codereview.chromium.org/1272833002/diff/160001/remoting/host/native_m...
File remoting/host/native_messaging/log_message_handler.cc (right):

https://codereview.chromium.org/1272833002/diff/160001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:19: // and when accessing
the weak pointer to the LogMessageHandler instance.
update the comment - it's not weak anymore.

https://codereview.chromium.org/1272833002/diff/160001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:20:
base::LazyInstance<base::Lock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
nit: maybe rename this to g_log_message_handler_lock to make it clear that it
needs to be locked to access g_log_message_handler.

https://codereview.chromium.org/1272833002/diff/160001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:23: // not being
intercepted. It must be dereferenced and updated under a lock.
s/a lock/the lock/

[Jamie, 2015-08-18 00:04:57]

The CQ bit was checked by jamiewalch@chromium.org

[Jamie, 2015-08-18 00:04:57]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1272833002/#ps180001
(title: "Review feedback.")

[Jamie, 2015-08-18 00:05:20]

FYI

https://codereview.chromium.org/1272833002/diff/160001/remoting/host/native_m...
File remoting/host/native_messaging/log_message_handler.cc (right):

https://codereview.chromium.org/1272833002/diff/160001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:19: // and when accessing
the weak pointer to the LogMessageHandler instance.
On 2015/08/17 23:06:24, Sergey Ulanov wrote:
> update the comment - it's not weak anymore.

Done.

https://codereview.chromium.org/1272833002/diff/160001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:20:
base::LazyInstance<base::Lock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
On 2015/08/17 23:06:25, Sergey Ulanov wrote:
> nit: maybe rename this to g_log_message_handler_lock to make it clear that it
> needs to be locked to access g_log_message_handler.

Done.

https://codereview.chromium.org/1272833002/diff/160001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.cc:23: // not being
intercepted. It must be dereferenced and updated under a lock.
On 2015/08/17 23:06:25, Sergey Ulanov wrote:
> s/a lock/the lock/

Done.

[commit-bot: I haz the power, 2015-08-18 00:06:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272833002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272833002/180001

[commit-bot: I haz the power, 2015-08-18 00:22:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-18 00:22:57]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Jamie, 2015-08-18 01:16:16]

The CQ bit was checked by jamiewalch@chromium.org

[Jamie, 2015-08-18 01:16:17]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1272833002/#ps200001
(title: "Fixed compilation on ChromeOS.")

[commit-bot: I haz the power, 2015-08-18 01:16:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272833002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272833002/200001

[commit-bot: I haz the power, 2015-08-18 02:11:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-18 02:11:29]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Jamie, 2015-08-18 19:11:48]

PTAL at the latest patch-set. I had to change some things to fix the unit tests
on Windows.

[Jamie, 2015-08-20 01:38:27]

Ping Sergey

[Sergey Ulanov, 2015-08-20 21:50:21]

still lgtm

https://codereview.chromium.org/1272833002/diff/220001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/1272833002/diff/220001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:292: if
(!result->GetString("type", &type) ||
Add a comment that we are skipping log messages here

https://codereview.chromium.org/1272833002/diff/220001/remoting/host/native_m...
File remoting/host/native_messaging/log_message_handler.h (right):

https://codereview.chromium.org/1272833002/diff/220001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.h:26: static const char*
debug_message_type_name;
kDebugMessageTypeName

[Jamie, 2015-08-20 22:29:06]

The CQ bit was checked by jamiewalch@chromium.org

[Jamie, 2015-08-20 22:29:06]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1272833002/#ps240001
(title: "Reviewer feedback.")

[Jamie, 2015-08-20 22:29:16]

FYI

https://codereview.chromium.org/1272833002/diff/220001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/1272833002/diff/220001/remoting/host/it2me/it...
remoting/host/it2me/it2me_native_messaging_host_unittest.cc:292: if
(!result->GetString("type", &type) ||
On 2015/08/20 21:50:20, Sergey Ulanov wrote:
> Add a comment that we are skipping log messages here

Done.

https://codereview.chromium.org/1272833002/diff/220001/remoting/host/native_m...
File remoting/host/native_messaging/log_message_handler.h (right):

https://codereview.chromium.org/1272833002/diff/220001/remoting/host/native_m...
remoting/host/native_messaging/log_message_handler.h:26: static const char*
debug_message_type_name;
On 2015/08/20 21:50:21, Sergey Ulanov wrote:
> kDebugMessageTypeName

Done.

[commit-bot: I haz the power, 2015-08-20 22:30:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1272833002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1272833002/240001

[commit-bot: I haz the power, 2015-08-21 00:11:30]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-08-21 00:12:43]

Patchset 13 (id:??) landed as
https://crrev.com/a1826488343b1923906c2b8bbd194eef51cfda26
Cr-Commit-Position: refs/heads/master@{#344622}