Implement browser-side logging to WebRtc log

Implement browser-side logging to WebRtc log

This adds a logging callback to RenderProcessHost and also adds logging
for device enumeration in the browser process.

BUG=332261
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247164

[vrk (LEFT CHROMIUM), 2014-01-17 02:08:28]

jam: render host stuff
grunell: everything

This is the "finished" version of Henrik's CL131793002. The actual logging is
being added in VideoCaptureManager and AudioInputDeviceManager. (There are still
some more log messages that needs to be added, but I figured I'd upload this CL
first)

[jam, 2014-01-17 05:47:44]

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1535:
base::Bind(webrtc_log_message_callback_, message));
how do we guarantee that the callback is valid on the IO thread? RPH is used on
the UI thread, so it's best if we can make the lifetime of the callback valid
just as long which means calling it on the UI thread

https://codereview.chromium.org/140843007/diff/70001/content/public/browser/r...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/140843007/diff/70001/content/public/browser/r...
content/public/browser/render_process_host.h:219: virtual void
SetWebRtcLogMessageCallback(
nit: add comment

https://codereview.chromium.org/140843007/diff/70001/content/public/browser/r...
content/public/browser/render_process_host.h:221: virtual void
WebRtcLogMessage(const std::string& message) = 0;
this is only called from inside content, so it should just be on
RenderProcessHostImpl only

[Henrik Grunell, 2014-01-17 08:51:25]

John, I think you had a comment in an offline mail thread about not having the
embedder register a logging callback/delegate in both browser and render
processes, but instead do IPC in the content layer and only register that in the
browser. I will do such a change in a separate follow-up CL (unless you think
it's fine as is).

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:143:
"AIDM::EODTT found the following devices:");
Same comments as in VideoCaptureManager for this and the next change.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1316: this,
device_thread_->message_loop_proxy().get());
Nit: AFAICT from the style guide, the code layout before this change was correct
but not after.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:192: // Adds
|message| to native logs for outstanding device requests.
Is sending the log to all outstanding requests' renderers what one would usually
want? I'm not particularly familiar with the MediaStream code in the browser
side, but it seems to me that we should log for the specific request this
message belongs to (if there is one, which I suppose there almost always is)?
Won't we spam logs with irrelevant information from other renderers this way?

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:296: void
DoAddLogMessage(const std::string& message);
Remove. Not implemented as a member function.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:442: "VCM::GADIODT
found the following devices:");
If there's no devices, will that be clear to the log reader? Should there be a
special log message for that?

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:464:
MediaStreamManager::AddLogMessage(device_info.name.GetNameAndModel() + "(" +
Perhaps " ("? (Space added)

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:465:
device_info.name.id());
End string with ")"?

[Jói, 2014-01-17 09:05:06]

+perkj to review the MediaStreamManager bits.

[perkj_chrome, 2014-01-20 11:07:07]

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:142:
MediaStreamManager::AddLogMessage(
Why do we have to log here? 
Why not do it in MediaStreamManager::DevicesEnumerated?

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1316: this,
device_thread_->message_loop_proxy().get());
On 2014/01/17 08:51:25, Henrik Grunell wrote:
> Nit: AFAICT from the style guide, the code layout before this change was
correct
> but not after.

I think Henrik is wrong and vrk is correct :->

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1405:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Here all device enumerations end up for both audio and video.
So if you actually wants to log enumeration results - I think this is the place.
Enumerations are triggered if no enumeration has previously been done or a
device has been plugged in or removed. 

As you know, this is not only used by webrtc.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:17: #include
"content/browser/renderer_host/media/media_stream_manager.h"
I think this include is the wrong direction.
If you actually want to log device enumeration and open / closing devices why
not only do it in MediaStreamManager.

[Henrik Grunell, 2014-01-20 16:23:07]

Just a little follow up on the formatting detail. :)

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1316: this,
device_thread_->message_loop_proxy().get());
On 2014/01/20 11:07:08, perkj wrote:
> On 2014/01/17 08:51:25, Henrik Grunell wrote:
> > Nit: AFAICT from the style guide, the code layout before this change was
> correct
> > but not after.
> 
> I think Henrik is wrong and vrk is correct :->

I don't have a very strong opinion on this, do what you think is best. (Follow
the file by all means.) But this is what I can see:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
(And personally I think the "before" version is easier to read, but that's
another thing.)

[vrk (LEFT CHROMIUM), 2014-01-22 02:25:41]

Addressed all comments, PTAL. 

Since the device enumeration logging could all be done in MediaStreamManager, I
added the camera open logging to this CL as well so that you can see the reason
for making MediaStreamManager::AddLogMessage public static. See
video_capture_controller.cc for the main changes.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:142:
MediaStreamManager::AddLogMessage(
On 2014/01/20 11:07:08, perkj wrote:
> Why do we have to log here? 
> Why not do it in MediaStreamManager::DevicesEnumerated? 

Sure, I can do that. I originally put more logging here but then moved it out of
this CL. Agree it makes more sense on MSM::DevicesEnumerated.

That said, I wanted to put an example of MediaStreamManager::AddLogMessage in
this CL. Device enumeration is not the only thing we want to log, but I was
going to put other log messages in separate CLs because they span all OSes
(didn't want to make the CL too big). So I added to this CL log messages on
video device error. PTAL.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:143:
"AIDM::EODTT found the following devices:");
On 2014/01/17 08:51:25, Henrik Grunell wrote:
> Same comments as in VideoCaptureManager for this and the next change.

Done.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1316: this,
device_thread_->message_loop_proxy().get());
On 2014/01/20 16:23:08, Henrik Grunell wrote:
> On 2014/01/20 11:07:08, perkj wrote:
> > On 2014/01/17 08:51:25, Henrik Grunell wrote:
> > > Nit: AFAICT from the style guide, the code layout before this change was
> > correct
> > > but not after.
> > 
> > I think Henrik is wrong and vrk is correct :->
> 
> I don't have a very strong opinion on this, do what you think is best. (Follow
> the file by all means.) But this is what I can see:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
> (And personally I think the "before" version is easier to read, but that's
> another thing.)

I believe either way is acceptable, but I reverted the change anyway. (FWIW I
didn't mean to make this style change; I had fiddled around with this call for
experimentation reasons and thought I reverted it back to the original code, but
apparently not :) )

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.cc:1405:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2014/01/20 11:07:08, perkj wrote:
> Here all device enumerations end up for both audio and video.
> So if you actually wants to log enumeration results - I think this is the
place.
> Enumerations are triggered if no enumeration has previously been done or a
> device has been plugged in or removed. 
> 
> As you know, this is not only used by webrtc.

OK, done.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:192: // Adds
|message| to native logs for outstanding device requests.
On 2014/01/17 08:51:25, Henrik Grunell wrote:
> Is sending the log to all outstanding requests' renderers what one would
usually
> want? I'm not particularly familiar with the MediaStream code in the browser
> side, but it seems to me that we should log for the specific request this
> message belongs to (if there is one, which I suppose there almost always is)?
> Won't we spam logs with irrelevant information from other renderers this way?

See implementation of AddLogMessageOnUIThread(). This code should send all
outstanding requests to each respective RenderProcessHost, which will send it to
the respective WebRtcLogHandlerHost. 

So for example: 
render process id 1 has native logging started, calls getUserMedia
render process id 2 does not have native logging started, callsGetUserMedia

render process id 1 will send a log message to RenderProcessHost 1, which
forwards it to WebRtcLogHandlerHost, which will add the log because it has
started.
render process id 2 will send a log message to RenderProcessHost 2, which
forwards it to WebRtcLogHandlerHost, which will ignore the log message because
native logging never began.

Do you agree? Let me know if you still see an issue.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:296: void
DoAddLogMessage(const std::string& message);
On 2014/01/17 08:51:25, Henrik Grunell wrote:
> Remove. Not implemented as a member function.

Thanks, done.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:17: #include
"content/browser/renderer_host/media/media_stream_manager.h"
On 2014/01/20 11:07:08, perkj wrote:
> I think this include is the wrong direction.
> If you actually want to log device enumeration and open / closing devices why
> not only do it in MediaStreamManager.

I was originally going to add more logging in a separate CL, but I added video
capture logging to this CL so you can better see the necessity for a static
logging method on MediaStreamManager. PTAL.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:442: "VCM::GADIODT
found the following devices:");
On 2014/01/17 08:51:25, Henrik Grunell wrote:
> If there's no devices, will that be clear to the log reader? Should there be a
> special log message for that?

Sure, added special log message for this case.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:464:
MediaStreamManager::AddLogMessage(device_info.name.GetNameAndModel() + "(" +
On 2014/01/17 08:51:25, Henrik Grunell wrote:
> Perhaps " ("? (Space added)

Done.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/video_capture_manager.cc:465:
device_info.name.id());
On 2014/01/17 08:51:25, Henrik Grunell wrote:
> End string with ")"?

Done.

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1535:
base::Bind(webrtc_log_message_callback_, message));
On 2014/01/17 05:47:44, jam wrote:
> how do we guarantee that the callback is valid on the IO thread? RPH is used
on
> the UI thread, so it's best if we can make the lifetime of the callback valid
> just as long which means calling it on the UI thread

OK, yeah that makes sense. Moved the thread hop to WebRtcLogMessageHost.

https://codereview.chromium.org/140843007/diff/70001/content/public/browser/r...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/140843007/diff/70001/content/public/browser/r...
content/public/browser/render_process_host.h:219: virtual void
SetWebRtcLogMessageCallback(
On 2014/01/17 05:47:44, jam wrote:
> nit: add comment

Done.

https://codereview.chromium.org/140843007/diff/70001/content/public/browser/r...
content/public/browser/render_process_host.h:221: virtual void
WebRtcLogMessage(const std::string& message) = 0;
On 2014/01/17 05:47:44, jam wrote:
> this is only called from inside content, so it should just be on
> RenderProcessHostImpl only

Done.

[jam, 2014-01-22 03:20:00]

lgtm for all files without "media" in the path, the rest I defer to others

https://codereview.chromium.org/140843007/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/140843007/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1524: void
RenderProcessHostImpl::SetWebRtcLogMessageCallback(
nit: we try to sort the methods in the cc file to match the order of the header,
so please put these methods in their order even if htat means multiple ifdefs in
the file

[vrk (LEFT CHROMIUM), 2014-01-22 04:38:06]

https://codereview.chromium.org/140843007/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/140843007/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1524: void
RenderProcessHostImpl::SetWebRtcLogMessageCallback(
On 2014/01/22 03:20:01, jam wrote:
> nit: we try to sort the methods in the cc file to match the order of the
header,
> so please put these methods in their order even if htat means multiple ifdefs
in
> the file

Done.

[Henrik Grunell, 2014-01-22 07:05:22]

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/140843007/diff/70001/content/browser/renderer...
content/browser/renderer_host/media/media_stream_manager.h:192: // Adds
|message| to native logs for outstanding device requests.
On 2014/01/22 02:25:42, Victoria Kirst wrote:
> On 2014/01/17 08:51:25, Henrik Grunell wrote:
> > Is sending the log to all outstanding requests' renderers what one would
> usually
> > want? I'm not particularly familiar with the MediaStream code in the browser
> > side, but it seems to me that we should log for the specific request this
> > message belongs to (if there is one, which I suppose there almost always
is)?
> > Won't we spam logs with irrelevant information from other renderers this
way?
> 
> See implementation of AddLogMessageOnUIThread(). This code should send all
> outstanding requests to each respective RenderProcessHost, which will send it
to
> the respective WebRtcLogHandlerHost. 
> 
> So for example: 
> render process id 1 has native logging started, calls getUserMedia
> render process id 2 does not have native logging started, callsGetUserMedia
> 
> render process id 1 will send a log message to RenderProcessHost 1, which
> forwards it to WebRtcLogHandlerHost, which will add the log because it has
> started.
> render process id 2 will send a log message to RenderProcessHost 2, which
> forwards it to WebRtcLogHandlerHost, which will ignore the log message because
> native logging never began.
> 
> Do you agree? Let me know if you still see an issue.

Let's discuss offline.

https://codereview.chromium.org/140843007/diff/300001/chrome/browser/media/we...
File chrome/browser/media/webrtc_logging_handler_host.cc (right):

https://codereview.chromium.org/140843007/diff/300001/chrome/browser/media/we...
chrome/browser/media/webrtc_logging_handler_host.cc:207:
BrowserThread::PostTask(
I don't think we should log if in STOPPING state, which is allowed in
OnAddLogMessage. It must be allowed from the renderer side though since messages
are buffered there. It's not a big deal however.

And no need to post a task if already on IO thread.

So I suggest post to same function if not on IO thread, and keep
if (logging_state_ == STARTED)
  LogToCircularBuffer(message);

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1405:
std::stringstream devices_msg;
I think someone told me in a CL not to use stringstream, but I can't find that
or anything in the style/code guides. So OK for me unless anyone else knows
more.

https://codereview.chromium.org/140843007/diff/300001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/140843007/diff/300001/content/public/browser/...
content/public/browser/render_process_host.h:219: // When set, |callback|
receives log messages regarding media devices
Maybe add "for example" before "media devices" to be more generic and future
proof.

[perkj_chrome, 2014-01-22 10:03:28]

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1405:
std::stringstream devices_msg;
On 2014/01/22 07:05:24, Henrik Grunell wrote:
> I think someone told me in a CL not to use stringstream, but I can't find that
> or anything in the style/code guides. So OK for me unless anyone else knows
> more.

Streams are only allowed for logging:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Streams
Maybe this is not really what the style guide means though.

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1502: for
(DeviceRequests::iterator it = requests_.begin(); it != requests_.end();
Are you aware that a request is "Pending" as long as the request has devices
opened? Ie- it only goes out of scope when the last device in the request has
been stopped.
(MediaStreamTrack::Stop() or MediaSTream::Stop()) or the page has been reloaded
etc..
Ie- if you do GetUserMedia twice - you will have two active requests and thus it
looks like you will send the same message twice to the same render process. 

Also- this will send the log to all render processes that have a MediaStream
running. Regardless if the tab has anything to do with the error.

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1506: // device
enumeration.
Can you add this comment to the header file as well?

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:190: static void
AddLogMessage(const std::string& message);
Please document where these log messages are sent and what they are used for.

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:433:
MediaStreamManager::AddLogMessage("Error on video capture: " + reason);
I think you should add an identifier - the session id in the call to
AddLogMessage. AddLogMessage can then find the request that contain devices with
that session id and only log to the render process that use the session id.

Note that a real device can be used in several tabs and thus have multiple
session ids. I am a bit lost of what VideoCaptureDeviceClient represents.

[vrk (LEFT CHROMIUM), 2014-01-22 18:30:29]

Responding to the major comments only right now. Per, let's chat about this f2f.
I think it might not be worth worrying about logging in the multiple tab case.
I'll set up a meeting for tomorrow morning PST, or ping me if you have time
tonight Stockholm-time.

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1502: for
(DeviceRequests::iterator it = requests_.begin(); it != requests_.end();
On 2014/01/22 10:03:29, perkj wrote:
> Are you aware that a request is "Pending" as long as the request has devices
> opened? Ie- it only goes out of scope when the last device in the request has
> been stopped.
> (MediaStreamTrack::Stop() or MediaSTream::Stop()) or the page has been
reloaded
> etc..
> Ie- if you do GetUserMedia twice - you will have two active requests and thus
it
> looks like you will send the same message twice to the same render process. 
> 
> Also- this will send the log to all render processes that have a MediaStream
> running. Regardless if the tab has anything to do with the error.

I think this behavior is acceptable in practice. We're really looking to add
logging to debug problems in the common and easy use case, which is one app in
one tab using WebRTC. Do you agree? If not, can you explain the scenario you're
concerned about, and the behavior you'd prefer?

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:433:
MediaStreamManager::AddLogMessage("Error on video capture: " + reason);
On 2014/01/22 10:03:29, perkj wrote:
> I think you should add an identifier - the session id in the call to
> AddLogMessage. AddLogMessage can then find the request that contain devices
with
> that session id and only log to the render process that use the session id.
> 
> Note that a real device can be used in several tabs and thus have multiple
> session ids. I am a bit lost of what VideoCaptureDeviceClient represents.

I'm not sure if it's worth doing this. Here's my reasoning: Look at how DVLOG is
used. There's nothing being done to disambiguate DVLOG messages between multiple
tabs/sessions, i.e. if there's an error for some camera X, it is logged to the
console without explaining which tab it came from. I don't think we need
anything more sophisticated than that for native logging.

[vrk (LEFT CHROMIUM), 2014-01-23 21:46:32]

Addressed all comments, PTAL. Chatted with Per offline and I looked deeper into
filtering by session id, but it still looks complex to me, so per our
conversation, I'll save that for a follow-up CL if we deem it worth the
complexity.

https://codereview.chromium.org/140843007/diff/300001/chrome/browser/media/we...
File chrome/browser/media/webrtc_logging_handler_host.cc (right):

https://codereview.chromium.org/140843007/diff/300001/chrome/browser/media/we...
chrome/browser/media/webrtc_logging_handler_host.cc:207:
BrowserThread::PostTask(
On 2014/01/22 07:05:24, Henrik Grunell wrote:
> I don't think we should log if in STOPPING state, which is allowed in
> OnAddLogMessage. It must be allowed from the renderer side though since
messages
> are buffered there. It's not a big deal however.

Done.

> And no need to post a task if already on IO thread.
>
> So I suggest post to same function if not on IO thread, and keep
> if (logging_state_ == STARTED)
>   LogToCircularBuffer(message);

FYI this pattern of

  void foo() {
    // The first time this method is called, we won't be
    // on the correct thread, so repost this same method 
    // to the desired thread. (this pattern is bad!)
    if (current != desired_thread) {
      desired_thread->PostTask(foo);
      return;
    }
    ... continue on desired_thread.
  }

is not a good pattern, as it makes it difficult to reason about threading (and
therefore easy to introduce threading bugs/hard to debug them). This pattern
used to be rampant in the <video>/<audio> code, but there was a big clean-up to
get rid of it. Talk to fischman@ or scherkus@ about it for more info. We should
do the same cleanup in the WebRTC code, and we shouldn't add more of it.

Also, for the same reason, the following is not a good pattern either:

   // Can be called on any thread
   void foo() {
     // If we're coincidentally on the thread we want to 
     // be on, continue without repost. (this pattern is bad!)
     if (current == desired_thread) {
        bar();
     } else {
        desired_thread->Post(bar);
     }
   }

The time saved by this check is not worth the difficulty in reasoning about the
threading story. There are other things we can do in follow up CLs too to make
the threading story here clearer. Can chat about this offline - fischman@ had
some suggestions.

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1405:
std::stringstream devices_msg;
On 2014/01/22 10:03:29, perkj wrote:
> On 2014/01/22 07:05:24, Henrik Grunell wrote:
> > I think someone told me in a CL not to use stringstream, but I can't find
that
> > or anything in the style/code guides. So OK for me unless anyone else knows
> > more.
> 
> Streams are only allowed for logging:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Streams
> Maybe this is not really what the style guide means though. 

Going to keep as-is. Let's fix this in a follow-up CL if need be.

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1502: for
(DeviceRequests::iterator it = requests_.begin(); it != requests_.end();
On 2014/01/22 18:30:30, Victoria Kirst wrote:
> On 2014/01/22 10:03:29, perkj wrote:
> > Are you aware that a request is "Pending" as long as the request has devices
> > opened? Ie- it only goes out of scope when the last device in the request
has
> > been stopped.
> > (MediaStreamTrack::Stop() or MediaSTream::Stop()) or the page has been
> reloaded
> > etc..
> > Ie- if you do GetUserMedia twice - you will have two active requests and
thus
> it
> > looks like you will send the same message twice to the same render process. 
> > 
> > Also- this will send the log to all render processes that have a MediaStream
> > running. Regardless if the tab has anything to do with the error.
> 
> I think this behavior is acceptable in practice. We're really looking to add
> logging to debug problems in the common and easy use case, which is one app in
> one tab using WebRTC. Do you agree? If not, can you explain the scenario
you're
> concerned about, and the behavior you'd prefer?

Talked offline. This is hard to fix and I think the behavior should be good
enough for now, so going to leave as-is and fix in follow-up CL if need be.

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1506: // device
enumeration.
On 2014/01/22 10:03:29, perkj wrote:
> Can you add this comment to the header file as well?

Done, though reworded.

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:190: static void
AddLogMessage(const std::string& message);
On 2014/01/22 10:03:29, perkj wrote:
> Please document where these log messages are sent and what they are used for.

Done. Let me know if you want more details.

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/140843007/diff/300001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:433:
MediaStreamManager::AddLogMessage("Error on video capture: " + reason);
On 2014/01/22 18:30:30, Victoria Kirst wrote:
> On 2014/01/22 10:03:29, perkj wrote:
> > I think you should add an identifier - the session id in the call to
> > AddLogMessage. AddLogMessage can then find the request that contain devices
> with
> > that session id and only log to the render process that use the session id.
> > 
> > Note that a real device can be used in several tabs and thus have multiple
> > session ids. I am a bit lost of what VideoCaptureDeviceClient represents.
> 
> I'm not sure if it's worth doing this. Here's my reasoning: Look at how DVLOG
is
> used. There's nothing being done to disambiguate DVLOG messages between
multiple
> tabs/sessions, i.e. if there's an error for some camera X, it is logged to the
> console without explaining which tab it came from. I don't think we need
> anything more sophisticated than that for native logging.

Talked offline. I looked into this, and I don't think I can implement this
without making a lot of plumbing changes. Since I don't think we really need
this, I'm going to keep the CL as-is and fix in follow-up CL if need be.

https://codereview.chromium.org/140843007/diff/300001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/140843007/diff/300001/content/public/browser/...
content/public/browser/render_process_host.h:219: // When set, |callback|
receives log messages regarding media devices
On 2014/01/22 07:05:24, Henrik Grunell wrote:
> Maybe add "for example" before "media devices" to be more generic and future
> proof.

Done.

[perkj_chrome, 2014-01-24 09:15:09]

lgtm with small nits and a recommendation to test with flash running in one tab
and webrtc in another.

I still think it would be nice if only the renderprocess that caused the logging
event to occur got notified but I realize that its not possible in all cases.

However- it is possible from all events that occur on a video capture device and
ends up in VideoCaptureController::VideoCaptureDevice::OnError since the
VideoCaptureDevice knows it session_id and the MediaStreamManager also knows the
devices and there session ids per request.
You might consider doing this in a follow up if you agree (or delegate the work
:->)

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1536: // Log the
message to all renderers that are requesting a MediaStream or
Can you expand the comment : that are requesting a MediaStream or have a
MediaStream running

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1539:
request->request_type == MEDIA_ENUMERATE_DEVICES) {
Fyi- MEDIA_ENUMERATE_DEVICES is called by flash for some reason every 10s or so.


I would recommend that you run flash in one tab and webrtc in another and look
at the logs to see that you are not spammed.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:191: static void
SendMessageToNativeLog(const std::string& message);
Can you add a comment that it is safe to call SendMessageToNativeLog on any
thread?
You are sending from an "unknown" thread in
VideoCaptureController::VideoCaptureDeviceClient::OnError".

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_device_impl.cc (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_device_impl.cc:193:
std::stringstream error_msg;
nit: Only create when an error occur and move to where its used.

[no longer working on chromium, 2014-01-24 09:54:37]

driven-by.

I have a strong concern on the change in MediaStreamManager, it is lots of new
code and I don't see why they have to be in MediaStreamManager. I am wondering
if it is possible to extract those code out into a new class (MediaDeviceLogger
or whatever), this will avoid making MSM more complicated, and also easier to
write unittest, and easier to maintain the thread safe of two classes.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:874: <<
it->device.id << ")" << std::endl;
btw, are you supposed to log the device_id? has the private concern been
addressed here?

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:876: if
(devices.empty())
nit, I think it is cleaner to do:
if (devices.empty()) {
  devices_msg << "No devices found.";
} else {
  for (StreamDeviceInfoArray::const_iterator it = devices.begin();
       it != devices.end(); ++it) {
    devices_msg << "  " << it->device.name << " (" << it->device.id << ")" <<
std::endl;
}

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1052: // Cache is
valid, so log the cached devices.
are you sure you want to log the cache for each SetupRequest?
Note that SetupRequest is used by all the clients, including pepper, and I know
that they call OpenDevice() once a while to poll the device. If things are still
the case, you will log a lot here.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1055:
LogDevicesToNativeLogs(video_type, video_enumeration_cache_.devices);
what do you log if the type is MEDIA_NO_SERVICE ?

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1524:
BrowserThread::UI, FROM_HERE,
I have some concern on the thread safety here. Previously MediaStreamManager is
listening to a WillDestroyCurrentMessageLoop() message to clean up its resource
when the IO thread is going away, then MediaStreamManager will be deleted on UI
thread.
But this method will post a task on UI thread to MediaStreamManager itself, can
it happen that MediaStreamManager gets deleted before the task is executed? I
think this is at least theoretically possible and depending on how this method
is used.

I believe you need to use a weak pointer to invalidate all the tasks when IO
thread is going away.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:191: static void
SendMessageToNativeLog(const std::string& message);
will this static API be used outside MediaStreamManager? if not, move it to
private.

[no longer working on chromium, 2014-01-24 10:36:17]

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1052: // Cache is
valid, so log the cached devices.
On 2014/01/24 09:54:38, xians1 wrote:
> are you sure you want to log the cache for each SetupRequest?
> Note that SetupRequest is used by all the clients, including pepper, and I
know
> that they call OpenDevice() once a while to poll the device. If things are
still
> the case, you will log a lot here.

Per pointed out that only media stream requests were logged in
AddLogMessageOnUIThread. But what I understand from the code is that you might
log the same request multiple times. Right?

[tommi (sloooow) - chröme, 2014-01-24 12:34:53]

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/desktop_capture_device.cc (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/desktop_capture_device.cc:168:
std::stringstream log("Failed to capture a frame.");
stringstream is overkill, just use std::string.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:42: static void
DoAddLogMessage(const std::string& message) {
I think //content prefers anonymous namespaces (even inside the content
namespace).  //media is a bit special in this way and that's the style you're
following here I think.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:874: <<
it->device.id << ")" << std::endl;
On 2014/01/24 09:54:38, xians1 wrote:
> btw, are you supposed to log the device_id? has the private concern been
> addressed here?

Agreed. To be safe we should either skip logging the ID or log only the ID as
javascript will see it (origin specific).

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1053:
SendMessageToNativeLog("Using cached devices for request.");
Would it make sense to aggregate all these messages into a single message so
that they're guaranteed to stay intact inside the log?

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1524:
BrowserThread::UI, FROM_HERE,
On 2014/01/24 09:54:38, xians1 wrote:
> I have some concern on the thread safety here. Previously MediaStreamManager
is
> listening to a WillDestroyCurrentMessageLoop() message to clean up its
resource
> when the IO thread is going away, then MediaStreamManager will be deleted on
UI
> thread.
> But this method will post a task on UI thread to MediaStreamManager itself,
can
> it happen that MediaStreamManager gets deleted before the task is executed? I
> think this is at least theoretically possible and depending on how this method
> is used.
> 
> I believe you need to use a weak pointer to invalidate all the tasks when IO
> thread is going away.

Actually, this method is static and the task that gets posted isn't bound to a
MediaStreamManager pointer.  DoAddLogMessage will actually do the checking of
whether the manager instance is dead or alive, so I think this is OK from that
standpoint.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:191: static void
SendMessageToNativeLog(const std::string& message);
+1 on documenting this function like the other methods.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_device_impl.cc (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_device_impl.cc:193:
std::stringstream error_msg;
On 2014/01/24 09:15:10, perkj wrote:
> nit: Only create when an error occur and move to where its used.

+1

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_device_impl.cc:258:
std::stringstream reason("Failed to start capture machine.");
std::string is all that is needed here.

[tommi (sloooow) - chröme, 2014-01-24 15:10:17]

Given that the biggest ask for me is simple to do (remove logging of device ids)
and that the other comments are also pretty simple (add comments etc), this
lgtm.

[tommi (sloooow) - chröme, 2014-01-24 15:11:13]

On 2014/01/24 15:10:17, tommi wrote:
> Given that the biggest ask for me is simple to do (remove logging of device
ids)
> and that the other comments are also pretty simple (add comments etc), this
> lgtm.

oh and to be clear, I'm assuming that my comments will be addressed before
landing of course :)

[Henrik Grunell, 2014-01-24 20:17:46]

LGTM if you fix outstanding comments by other reviewers.

[vrk (LEFT CHROMIUM), 2014-01-25 00:50:20]

Thanks for reviewing! Addressed all comments.

Per - I tested running a pepper flash webcam website concurrently with a website
using native logging in a different tab, and the log is not spammed with pepper
content. I agree that we should be able to filter device errors to the process
requesting the logging in the first place, though I think this will take some
refactoring of MediaStreamManager to do cleanly. We have crbug.com/333894 and
crbug.com/337982, and I want to spend some time next week planning the next
steps.

Shijing - This code needs to be in MediaStreamManager because MediaStreamManager
tracks which Render processes are requesting devices, and that's exactly what
information we need in order to send logs to the render processes requesting
native logging. I agree the current design is not ideal, but I think this is
also in part due to MediaStreamManager needing a redesign. As I mention to Per
above, we have two bugs, crbug.com/333894 for revisiting WebRTC logging and
crbug.com/337982 for revisiting MediaStreamManager, and we should talk about the
next steps next week.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/desktop_capture_device.cc (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/desktop_capture_device.cc:168:
std::stringstream log("Failed to capture a frame.");
On 2014/01/24 12:34:54, tommi wrote:
> stringstream is overkill, just use std::string.

Done, here and elsewhere.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:42: static void
DoAddLogMessage(const std::string& message) {
On 2014/01/24 12:34:54, tommi wrote:
> I think //content prefers anonymous namespaces (even inside the content
> namespace).  //media is a bit special in this way and that's the style you're
> following here I think.

Done.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:874: <<
it->device.id << ")" << std::endl;
On 2014/01/24 09:54:38, xians1 wrote:
> btw, are you supposed to log the device_id? has the private concern been
> addressed here?

Removed. (Fwiw I also sent a mail to kaichou@ with sample logs to get the
privacy OK.)

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:874: <<
it->device.id << ")" << std::endl;
On 2014/01/24 12:34:54, tommi wrote:
> On 2014/01/24 09:54:38, xians1 wrote:
> > btw, are you supposed to log the device_id? has the private concern been
> > addressed here?
> 
> Agreed. To be safe we should either skip logging the ID or log only the ID as
> javascript will see it (origin specific).

Done, just omitted.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:876: if
(devices.empty())
On 2014/01/24 09:54:38, xians1 wrote:
> nit, I think it is cleaner to do:
> if (devices.empty()) {
>   devices_msg << "No devices found.";
> } else {
>   for (StreamDeviceInfoArray::const_iterator it = devices.begin();
>        it != devices.end(); ++it) {
>     devices_msg << "  " << it->device.name << " (" << it->device.id << ")" <<
> std::endl;
> }

Done.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1052: // Cache is
valid, so log the cached devices.
On 2014/01/24 09:54:38, xians1 wrote:
> are you sure you want to log the cache for each SetupRequest?
> Note that SetupRequest is used by all the clients, including pepper, and I
know
> that they call OpenDevice() once a while to poll the device. If things are
still
> the case, you will log a lot here.

Thanks for letting me know. I don't think there will be many situations in which
people are running native logging and Pepper at the same time, but added an if
statement to limit to MediaStream requests. This isn't a great solution, but I
think the real problem is that MediaStreamManager needs to be refactored. At the
very least, I don't think pepper code should be in this class. I added
crbug.com/337982 for this cleanup.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1055:
LogDevicesToNativeLogs(video_type, video_enumeration_cache_.devices);
On 2014/01/24 09:54:38, xians1 wrote:
> what do you log if the type is MEDIA_NO_SERVICE ?

See LogDevicesToNativeLogs - it would log the MEDIA_NO_SERVICE enum value and
"No devices found." I don't think that's the worst thing in the world, but I
added some logic to make it clearer.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1524:
BrowserThread::UI, FROM_HERE,
On 2014/01/24 12:34:54, tommi wrote:
> On 2014/01/24 09:54:38, xians1 wrote:
> > I have some concern on the thread safety here. Previously MediaStreamManager
> is
> > listening to a WillDestroyCurrentMessageLoop() message to clean up its
> resource
> > when the IO thread is going away, then MediaStreamManager will be deleted on
> UI
> > thread.
> > But this method will post a task on UI thread to MediaStreamManager itself,
> can
> > it happen that MediaStreamManager gets deleted before the task is executed?
I
> > think this is at least theoretically possible and depending on how this
method
> > is used.
> > 
> > I believe you need to use a weak pointer to invalidate all the tasks when IO
> > thread is going away.
> 
> Actually, this method is static and the task that gets posted isn't bound to a
> MediaStreamManager pointer.  DoAddLogMessage will actually do the checking of
> whether the manager instance is dead or alive, so I think this is OK from that
> standpoint.

I believe so as well, so leaving as is.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1536: // Log the
message to all renderers that are requesting a MediaStream or
On 2014/01/24 09:15:10, perkj wrote:
> Can you expand the comment : that are requesting a MediaStream or have a
> MediaStream running

Done.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1539:
request->request_type == MEDIA_ENUMERATE_DEVICES) {
On 2014/01/24 09:15:10, perkj wrote:
> Fyi- MEDIA_ENUMERATE_DEVICES is called by flash for some reason every 10s or
so.
Pepper flash? I don't understand why MediaStreamManager deals with Pepper
requests at all. I created crbug.com/337982 in response to this.
 
> I would recommend that you run flash in one tab and webrtc in another and look
> at the logs to see that you are not spammed.

I just removed the MEDIA_ENUMERATE_DEVICES if statement. We really only care
about MEDIA_GENERATE_STREAM.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:191: static void
SendMessageToNativeLog(const std::string& message);
On 2014/01/24 12:34:54, tommi wrote:
> +1 on documenting this function like the other methods.

Done.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:191: static void
SendMessageToNativeLog(const std::string& message);
On 2014/01/24 09:15:10, perkj wrote:
> Can you add a comment that it is safe to call SendMessageToNativeLog on any
> thread?
> You are sending from an "unknown" thread in
> VideoCaptureController::VideoCaptureDeviceClient::OnError".

Done.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:191: static void
SendMessageToNativeLog(const std::string& message);
On 2014/01/24 09:54:38, xians1 wrote:
> will this static API be used outside MediaStreamManager? if not, move it to
> private.

Yes, it's already used in VideoCaptureController::VideoCaptureDeviceClient in
this CL. (Btw, an easy way to check is to view the raw patch (click [raw] link
next to Download) and search for ::SendMessageToNativeLog to search the whole
CL). Also, the point of this method is to allow logging from any class in the
browser process in chrome/ or content/.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_device_impl.cc (right):

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_device_impl.cc:193:
std::stringstream error_msg;
On 2014/01/24 12:34:54, tommi wrote:
> On 2014/01/24 09:15:10, perkj wrote:
> > nit: Only create when an error occur and move to where its used.
> 
> +1

Done.

https://codereview.chromium.org/140843007/diff/910001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_device_impl.cc:258:
std::stringstream reason("Failed to start capture machine.");
On 2014/01/24 12:34:54, tommi wrote:
> std::string is all that is needed here.

Done.

[commit-bot: I haz the power, 2014-01-25 01:59:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/140843007/1200001

[commit-bot: I haz the power, 2014-01-25 02:51:03]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2014-01-25 06:34:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/140843007/1200001

[commit-bot: I haz the power, 2014-01-25 07:15:25]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-25 12:06:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/140843007/1200001

[commit-bot: I haz the power, 2014-01-25 12:48:45]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2014-01-25 19:13:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/140843007/140002

[commit-bot: I haz the power, 2014-01-25 20:01:38]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2014-01-25 21:06:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/140843007/1710003

[commit-bot: I haz the power, 2014-01-26 10:05:32]

Change committed as 247164

[Henrik Grunell, 2014-01-27 08:33:17]

Just two questions to follow up on this CL.

Also made a CL with corrections of some comments:
https://codereview.chromium.org/148183002/

https://codereview.chromium.org/140843007/diff/1710003/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/140843007/diff/1710003/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:122:
media_stream_manager()->AddLogMessageOnUIThread(message);
Is msm guaranteed to always be valid here?

https://codereview.chromium.org/140843007/diff/1710003/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:1546: // TODO(vrk):
Figure out what's going on here and fix.
why not use
#if defined(ENABLE_WEBRTC)
? :)

Should probably wrap more of the code added in this CL inside that define.

[Henrik Grunell, 2014-02-05 11:29:17]

On 2014/01/27 08:33:17, Henrik Grunell wrote:
> Just two questions to follow up on this CL.
> 
> Also made a CL with corrections of some comments:
> https://codereview.chromium.org/148183002/
> 
>
https://codereview.chromium.org/140843007/diff/1710003/content/browser/render...
> File content/browser/renderer_host/media/media_stream_manager.cc (right):
> 
>
https://codereview.chromium.org/140843007/diff/1710003/content/browser/render...
> content/browser/renderer_host/media/media_stream_manager.cc:122:
> media_stream_manager()->AddLogMessageOnUIThread(message);
> Is msm guaranteed to always be valid here?
> 
>
https://codereview.chromium.org/140843007/diff/1710003/content/browser/render...
> content/browser/renderer_host/media/media_stream_manager.cc:1546: //
TODO(vrk):
> Figure out what's going on here and fix.
> why not use
> #if defined(ENABLE_WEBRTC)
> ? :)
> 
> Should probably wrap more of the code added in this CL inside that define.

Ping Victoria. Can you follow up on these two questions?