Moving WebRTC logging related files from content to chrome.

Moving WebRTC logging related files from content to chrome.

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

[Henrik Grunell, 2013-05-22 12:43:36]

Hi, per jam@'s request, I've moved all WebRTC logging related files from content
to chrome.

The main change besides actually moving the files is how the IPC filters are
registered and enabling the log in MediaStreamDependencyFactory.

Feel free to review the whole package, or the minimum as owner:
Scott: chrome/*
Avi: content/*
Chris: chrome/common/media/webrtc_logging_messages.h
Sergey: Dep on libjingle in chrome/renderer/DEPS

[Avi (use Gerrit), 2013-05-22 14:39:57]

Sure, mod nits. lgtm

https://codereview.chromium.org/15741003/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/15741003/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:21: //#include
"content/renderer/media/webrtc_logging_message_filter.h"
don't comment out lines

[sky, 2013-05-22 16:19:32]

https://codereview.chromium.org/15741003/diff/1/chrome/browser/media/webrtc_l...
File chrome/browser/media/webrtc_logging_handler_host.h (right):

https://codereview.chromium.org/15741003/diff/1/chrome/browser/media/webrtc_l...
chrome/browser/media/webrtc_logging_handler_host.h:12: namespace chrome {
note: its fine to use the chrome namespace here but don't feel you have to.

https://codereview.chromium.org/15741003/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/15741003/diff/1/chrome/chrome_browser.gypi#ne...
chrome/chrome_browser.gypi:3042: 'browser/media/webrtc_logging_handler_host.cc',
See comment on line 76. In general we list file with all and then exclude where
not needed.

https://codereview.chromium.org/15741003/diff/1/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/15741003/diff/1/chrome/chrome_common.gypi#new...
chrome/chrome_common.gypi:352: 'common/media/webrtc_logging_messages.h',
Exclude when wrt not enabled?

https://codereview.chromium.org/15741003/diff/1/chrome/chrome_renderer.gypi
File chrome/chrome_renderer.gypi (right):

https://codereview.chromium.org/15741003/diff/1/chrome/chrome_renderer.gypi#n...
chrome/chrome_renderer.gypi:356:
'renderer/media/webrtc_logging_handler_impl.cc',
Same comment as chrome_browser.gypi

https://codereview.chromium.org/15741003/diff/1/chrome/common/common_message_...
File chrome/common/common_message_generator.h (right):

https://codereview.chromium.org/15741003/diff/1/chrome/common/common_message_...
chrome/common/common_message_generator.h:11: #include
"chrome/common/media/webrtc_logging_messages.h"
Only include when rtc enabled?

[Sergey Ulanov, 2013-05-22 17:48:25]

libjingle in DEPS - lgtm

[Chris Evans, 2013-05-22 19:13:32]

On 2013/05/22 17:48:25, Sergey Ulanov wrote:
> libjingle in DEPS - lgtm

LGTM

[Henrik Grunell, 2013-05-23 12:50:11]

Made code review fixes. The rest of the changes are due to rebase.

One thing that doesn't work is a shared build. g_logging_delegate in
third_party/libjingle/overrides/talk/base/logging.cc is set in chrome (by
calling InitDiagnosticLoggingDelegate), and when used in content (by libjingle)
it's NULL.

Exporting g_logging_delegate works for me, but I guess there's still two copies
of the variable and we can get a regression in the future. One solution is to
add a public interface in content to that the calls goes through, so that
setting g_logging_delegate is done in content.

What do you think?

https://chromiumcodereview.appspot.com/15741003/diff/1/chrome/browser/media/w...
File chrome/browser/media/webrtc_logging_handler_host.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/1/chrome/browser/media/w...
chrome/browser/media/webrtc_logging_handler_host.h:12: namespace chrome {
On 2013/05/22 16:19:32, sky wrote:
> note: its fine to use the chrome namespace here but don't feel you have to.

OK.

https://chromiumcodereview.appspot.com/15741003/diff/1/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/15741003/diff/1/chrome/chrome_browser....
chrome/chrome_browser.gypi:3042: 'browser/media/webrtc_logging_handler_host.cc',
On 2013/05/22 16:19:32, sky wrote:
> See comment on line 76. In general we list file with all and then exclude
where
> not needed.

Done.

https://chromiumcodereview.appspot.com/15741003/diff/1/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://chromiumcodereview.appspot.com/15741003/diff/1/chrome/chrome_common.g...
chrome/chrome_common.gypi:352: 'common/media/webrtc_logging_messages.h',
On 2013/05/22 16:19:32, sky wrote:
> Exclude when wrt not enabled?

Yes. Done.

https://chromiumcodereview.appspot.com/15741003/diff/1/chrome/chrome_renderer...
File chrome/chrome_renderer.gypi (right):

https://chromiumcodereview.appspot.com/15741003/diff/1/chrome/chrome_renderer...
chrome/chrome_renderer.gypi:356:
'renderer/media/webrtc_logging_handler_impl.cc',
On 2013/05/22 16:19:32, sky wrote:
> Same comment as chrome_browser.gypi

Done.

https://chromiumcodereview.appspot.com/15741003/diff/1/chrome/common/common_m...
File chrome/common/common_message_generator.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/1/chrome/common/common_m...
chrome/common/common_message_generator.h:11: #include
"chrome/common/media/webrtc_logging_messages.h"
On 2013/05/22 16:19:32, sky wrote:
> Only include when rtc enabled?

Yes. Done.

https://chromiumcodereview.appspot.com/15741003/diff/1/content/renderer/media...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://chromiumcodereview.appspot.com/15741003/diff/1/content/renderer/media...
content/renderer/media/media_stream_dependency_factory.cc:21: //#include
"content/renderer/media/webrtc_logging_message_filter.h"
On 2013/05/22 14:39:57, Avi wrote:
> don't comment out lines

Oops, missed that. Thanks. Done.

[Henrik Grunell, 2013-05-23 13:31:12]

On 2013/05/23 12:50:11, Henrik Grunell wrote:

> One thing that doesn't work is a shared build. g_logging_delegate in
> third_party/libjingle/overrides/talk/base/logging.cc is set in chrome (by
> calling InitDiagnosticLoggingDelegate), and when used in content (by
libjingle)
> it's NULL.
> 
> Exporting g_logging_delegate works for me, but I guess there's still two
copies
> of the variable and we can get a regression in the future. One solution is to
> add a public interface in content to that the calls goes through, so that
> setting g_logging_delegate is done in content.
> 
> What do you think?

Added this in patch 3. Have a look. (Verified it works too.)

[Jói, 2013-05-23 20:40:15]

Hi Henrik,

Sorry about the late response, I had a busy morning.  I will try to
make sure to respond to whatever feedback you have early tomorrow
morning (Pacific time) before I hop on my plane back home.  Starting
Saturday I'll be back in Iceland, so much less time difference.

The content::InitWebRtcLoggingDelegate approach to fixing libjingle's
g_logging_delegate seems incomplete; if you were to now use libjingle
from //chrome, it seems to me that this would fail, as you've only
initialized the variable on the //content side.  A proper fix for
using libjingle across multiple modules in a components build seems to
be to change libjingle to itself be a 'type': '<(component)' in its
.gyp file, and export this global, that way there's just one instance
of it.  Not sure if there are other viable options; perhaps the
current workaround is OK for now.

I'm curious about the ContentRendererClient::InitWebRtcLogging method.
 This is used only by MediaStreamDependencyFactory, as far as I can
tell, and that class seems to have some knowledge of logging (e.g., it
has a webrtc_log_open_ member and a constraint that seems to turn on
logging.  If we are moving logging out of //content, is it also
possible to move this bit out of //content?  That would obviate the
need to add a method to ContentRendererClient and perhaps simplify
things a bit?

Cheers,
Jói


On Thu, May 23, 2013 at 6:31 AM,  <grunell@chromium.org> wrote:
> On 2013/05/23 12:50:11, Henrik Grunell wrote:
>
>> One thing that doesn't work is a shared build. g_logging_delegate in
>> third_party/libjingle/overrides/talk/base/logging.cc is set in chrome (by
>> calling InitDiagnosticLoggingDelegate), and when used in content (by
>
> libjingle)
>>
>> it's NULL.
>
>
>> Exporting g_logging_delegate works for me, but I guess there's still two
>
> copies
>>
>> of the variable and we can get a regression in the future. One solution is
>> to
>> add a public interface in content to that the calls goes through, so that
>> setting g_logging_delegate is done in content.
>
>
>> What do you think?
>
>
> Added this in patch 3. Have a look. (Verified it works too.)
>
> https://chromiumcodereview.appspot.com/15741003/

[jam, 2013-05-24 08:28:46]

+1 to both of Joi's points. can we avoid having any changes at all in content
related to this. if target types need to change to support this for component
build, that should be done instead of the current approach

[tommi (sloooow) - chröme, 2013-05-24 10:32:31]

Changing libjingle to support shared_library:
  TL;DR: Making that change would be nice, but it will take time and is not
necessary for this CL.

  This would be nice and I have looked into it in the past, but it's not a
trivial change.  Libjingle is a "third party" library that has both a public and
private repository versions (that are kept in sync).  It has always been
developed and consumed as a lib and as such does not have any export directives
such as what we use for content, base, media etc.  Making any changes in
libjingle takes a long time and although we could spend time on exporting all
the types, classes etc from libjingle to support this, there are no bots in
libjingle either internally or externally that would catch regressions on a
per-cl basis, which would make rolls into Chrome much more problematic.

As is, only content uses libjingle and we should avoid adding a dependency on
libjingle to other parts of Chrome (such as //chrome).

To allow chrome to set the global logging pointer, I think that a method to
content's API has to be added.  Exporting the global pointer is just a hacky way
to circumvent modifying the interface and is relying on an implementation detail
of the libjingle lib (i.e. exporting the lib's global variable from content).

Remove InitWebRtcLogging?
  Yes, I think we can do that.  We can make a change to libjingle's
PeerConnection construction code and call the log handler (if already set by
chrome), and tell it to initialize the log.

All that being said, as far as I can see we still need a single public method in
content that allows chrome to set its log handler.

[Henrik Grunell, 2013-05-24 11:30:21]

On 2013/05/24 10:32:31, tommi wrote:
> Changing libjingle to support shared_library:
>   TL;DR: Making that change would be nice, but it will take time and is not
> necessary for this CL.
> 
>   This would be nice and I have looked into it in the past, but it's not a
> trivial change.  Libjingle is a "third party" library that has both a public
and
> private repository versions (that are kept in sync).  It has always been
> developed and consumed as a lib and as such does not have any export
directives
> such as what we use for content, base, media etc.  Making any changes in
> libjingle takes a long time and although we could spend time on exporting all
> the types, classes etc from libjingle to support this, there are no bots in
> libjingle either internally or externally that would catch regressions on a
> per-cl basis, which would make rolls into Chrome much more problematic.
> 
> As is, only content uses libjingle and we should avoid adding a dependency on
> libjingle to other parts of Chrome (such as //chrome).
> 
> To allow chrome to set the global logging pointer, I think that a method to
> content's API has to be added.  Exporting the global pointer is just a hacky
way
> to circumvent modifying the interface and is relying on an implementation
detail
> of the libjingle lib (i.e. exporting the lib's global variable from content).
> 
> Remove InitWebRtcLogging?
>   Yes, I think we can do that.  We can make a change to libjingle's
> PeerConnection construction code and call the log handler (if already set by
> chrome), and tell it to initialize the log.
> 
> All that being said, as far as I can see we still need a single public method
in
> content that allows chrome to set its log handler.

Thanks for commenting on this. Nothing further to add from me at this point.

[tommi (sloooow) - chröme, 2013-05-24 16:27:49]

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/DE...
chrome/renderer/DEPS:24: "+third_party/libjingle",
Hej Henrik - I think we should revert this.  Chrome shouldn't depend on
libjingle.  #including stuff from the overrides/ folder is a gray area though
since strictly speaking it "may" not be a part of libjingle itself (such as in
the case of the header you're including).

[Henrik Grunell, 2013-05-24 18:26:01]

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/DE...
chrome/renderer/DEPS:24: "+third_party/libjingle",
On 2013/05/24 16:27:49, tommi wrote:
> Hej Henrik - I think we should revert this.  Chrome shouldn't depend on
> libjingle.  #including stuff from the overrides/ folder is a gray area though
> since strictly speaking it "may" not be a part of libjingle itself (such as in
> the case of the header you're including).

Agree. I'll revert this when we have settled on the other details and I make the
changes for a new patch.

[Jói, 2013-05-24 20:47:29]

I took another brief look, with the latest details from Henrik and Tommi in
mind.  In general this looks OK to me, would like to get rid of the addition to
ContentRendererClient though, and this seems doable.

I think jam@ will need to have the final say here.

Cheers,
Jói

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/ch...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/ch...
chrome/renderer/chrome_content_renderer_client.h:131: virtual void
InitWebRtcLogging(const std::string& app_session_id) OVERRIDE;
As discussed, if this can be removed (both here and from the
content::ContentRendererClient interface) I think that would be better.

[Henrik Grunell, 2013-05-27 16:23:06]

- Removed client API.
- Moved parsing of constraint to libjingle.
- Also rebase changes.

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/DE...
chrome/renderer/DEPS:24: "+third_party/libjingle",
On 2013/05/24 18:26:02, Henrik Grunell wrote:
> On 2013/05/24 16:27:49, tommi wrote:
> > Hej Henrik - I think we should revert this.  Chrome shouldn't depend on
> > libjingle.  #including stuff from the overrides/ folder is a gray area
though
> > since strictly speaking it "may" not be a part of libjingle itself (such as
in
> > the case of the header you're including).
> 
> Agree. I'll revert this when we have settled on the other details and I make
the
> changes for a new patch.

Actually, the handler includes the delegate interface since it implements it. So
I've kept it, but it would be nice to not have this. The alternative I see is to
wrap the interface in content but that's not very nice either.

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/ch...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/ch...
chrome/renderer/chrome_content_renderer_client.h:131: virtual void
InitWebRtcLogging(const std::string& app_session_id) OVERRIDE;
On 2013/05/24 20:47:29, Jói wrote:
> As discussed, if this can be removed (both here and from the
> content::ContentRendererClient interface) I think that would be better.

Done.

[Jói, 2013-05-27 17:50:03]

I think this is looking pretty good, with some nits and questions below.

You'll need jam@ to sign off.

Cheers,
Jói

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/DE...
chrome/renderer/DEPS:24: "+third_party/libjingle",
On 2013/05/27 16:23:06, Henrik Grunell wrote:
> On 2013/05/24 18:26:02, Henrik Grunell wrote:
> > On 2013/05/24 16:27:49, tommi wrote:
> > > Hej Henrik - I think we should revert this.  Chrome shouldn't depend on
> > > libjingle.  #including stuff from the overrides/ folder is a gray area
> though
> > > since strictly speaking it "may" not be a part of libjingle itself (such
as
> in
> > > the case of the header you're including).
> > 
> > Agree. I'll revert this when we have settled on the other details and I make
> the
> > changes for a new patch.
> 
> Actually, the handler includes the delegate interface since it implements it.
So
> I've kept it, but it would be nice to not have this. The alternative I see is
to
> wrap the interface in content but that's not very nice either.

I'd suggest at least narrowing it down to just the one header you actually need.

https://chromiumcodereview.appspot.com/15741003/diff/49006/chrome/browser/med...
File chrome/browser/media/webrtc_logging_handler_host.cc (right):

https://chromiumcodereview.appspot.com/15741003/diff/49006/chrome/browser/med...
chrome/browser/media/webrtc_logging_handler_host.cc:35:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
nit: Could put a [ using content::BrowserThread; ] near the top to avoid the
several repetitions of content::BrowserThread in this file.

https://chromiumcodereview.appspot.com/15741003/diff/49006/chrome/renderer/me...
File chrome/renderer/media/webrtc_logging_handler_impl.cc (right):

https://chromiumcodereview.appspot.com/15741003/diff/49006/chrome/renderer/me...
chrome/renderer/media/webrtc_logging_handler_impl.cc:84: // TODO(grunell):
Implement.
Is this not needed for the current change?

https://chromiumcodereview.appspot.com/15741003/diff/49006/content/public/ren...
File content/public/renderer/webrtc_logging_initializer.cc (right):

https://chromiumcodereview.appspot.com/15741003/diff/49006/content/public/ren...
content/public/renderer/webrtc_logging_initializer.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
nit: No (c). I think with or without is OK though, but the canonical version now
is without (c).

[Henrik Grunell, 2013-05-28 06:04:29]

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/DE...
chrome/renderer/DEPS:24: "+third_party/libjingle",
On 2013/05/27 17:50:03, Jói wrote:
> On 2013/05/27 16:23:06, Henrik Grunell wrote:
> > On 2013/05/24 18:26:02, Henrik Grunell wrote:
> > > On 2013/05/24 16:27:49, tommi wrote:
> > > > Hej Henrik - I think we should revert this.  Chrome shouldn't depend on
> > > > libjingle.  #including stuff from the overrides/ folder is a gray area
> > though
> > > > since strictly speaking it "may" not be a part of libjingle itself (such
> as
> > in
> > > > the case of the header you're including).
> > > 
> > > Agree. I'll revert this when we have settled on the other details and I
make
> > the
> > > changes for a new patch.
> > 
> > Actually, the handler includes the delegate interface since it implements
it.
> So
> > I've kept it, but it would be nice to not have this. The alternative I see
is
> to
> > wrap the interface in content but that's not very nice either.
> 
> I'd suggest at least narrowing it down to just the one header you actually
need.

Of course. Done.

https://chromiumcodereview.appspot.com/15741003/diff/49006/chrome/browser/med...
File chrome/browser/media/webrtc_logging_handler_host.cc (right):

https://chromiumcodereview.appspot.com/15741003/diff/49006/chrome/browser/med...
chrome/browser/media/webrtc_logging_handler_host.cc:35:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
On 2013/05/27 17:50:03, Jói wrote:
> nit: Could put a [ using content::BrowserThread; ] near the top to avoid the
> several repetitions of content::BrowserThread in this file.

Done.

https://chromiumcodereview.appspot.com/15741003/diff/49006/chrome/renderer/me...
File chrome/renderer/media/webrtc_logging_handler_impl.cc (right):

https://chromiumcodereview.appspot.com/15741003/diff/49006/chrome/renderer/me...
chrome/renderer/media/webrtc_logging_handler_impl.cc:84: // TODO(grunell):
Implement.
On 2013/05/27 17:50:03, Jói wrote:
> Is this not needed for the current change?

No it isn't.

https://chromiumcodereview.appspot.com/15741003/diff/49006/content/public/ren...
File content/public/renderer/webrtc_logging_initializer.cc (right):

https://chromiumcodereview.appspot.com/15741003/diff/49006/content/public/ren...
content/public/renderer/webrtc_logging_initializer.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2013/05/27 17:50:03, Jói wrote:
> nit: No (c). I think with or without is OK though, but the canonical version
now
> is without (c).

Done.

[Jói, 2013-05-28 10:48:22]

LGTM, please get a final sign-off from jam@

On Tue, May 28, 2013 at 7:04 AM,  <grunell@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/DEPS
> File chrome/renderer/DEPS (right):
>
>
https://chromiumcodereview.appspot.com/15741003/diff/33001/chrome/renderer/DE...
> chrome/renderer/DEPS:24: "+third_party/libjingle",
> On 2013/05/27 17:50:03, Jói wrote:
>>
>> On 2013/05/27 16:23:06, Henrik Grunell wrote:
>> > On 2013/05/24 18:26:02, Henrik Grunell wrote:
>> > > On 2013/05/24 16:27:49, tommi wrote:
>> > > > Hej Henrik - I think we should revert this.  Chrome shouldn't
>
> depend on
>>
>> > > > libjingle.  #including stuff from the overrides/ folder is a
>
> gray area
>>
>> > though
>> > > > since strictly speaking it "may" not be a part of libjingle
>
> itself (such
>>
>> as
>> > in
>> > > > the case of the header you're including).
>> > >
>> > > Agree. I'll revert this when we have settled on the other details
>
> and I make
>>
>> > the
>> > > changes for a new patch.
>> >
>> > Actually, the handler includes the delegate interface since it
>
> implements it.
>>
>> So
>> > I've kept it, but it would be nice to not have this. The alternative
>
> I see is
>>
>> to
>> > wrap the interface in content but that's not very nice either.
>
>
>> I'd suggest at least narrowing it down to just the one header you
>
> actually need.
>
> Of course. Done.
>
>
>
https://chromiumcodereview.appspot.com/15741003/diff/49006/chrome/browser/med...
> File chrome/browser/media/webrtc_logging_handler_host.cc (right):
>
>
https://chromiumcodereview.appspot.com/15741003/diff/49006/chrome/browser/med...
> chrome/browser/media/webrtc_logging_handler_host.cc:35:
> DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
> On 2013/05/27 17:50:03, Jói wrote:
>>
>> nit: Could put a [ using content::BrowserThread; ] near the top to
>
> avoid the
>>
>> several repetitions of content::BrowserThread in this file.
>
>
> Done.
>
>
>
https://chromiumcodereview.appspot.com/15741003/diff/49006/chrome/renderer/me...
> File chrome/renderer/media/webrtc_logging_handler_impl.cc (right):
>
>
https://chromiumcodereview.appspot.com/15741003/diff/49006/chrome/renderer/me...
> chrome/renderer/media/webrtc_logging_handler_impl.cc:84: //
> TODO(grunell): Implement.
> On 2013/05/27 17:50:03, Jói wrote:
>>
>> Is this not needed for the current change?
>
>
> No it isn't.
>
>
>
https://chromiumcodereview.appspot.com/15741003/diff/49006/content/public/ren...
> File content/public/renderer/webrtc_logging_initializer.cc (right):
>
>
https://chromiumcodereview.appspot.com/15741003/diff/49006/content/public/ren...
> content/public/renderer/webrtc_logging_initializer.cc:1: // Copyright
> (c) 2013 The Chromium Authors. All rights reserved.
> On 2013/05/27 17:50:03, Jói wrote:
>>
>> nit: No (c). I think with or without is OK though, but the canonical
>
> version now
>>
>> is without (c).
>
>
> Done.
>
> https://chromiumcodereview.appspot.com/15741003/

[sky, 2013-05-28 15:38:09]

chrome side LGTM

[Henrik Grunell, 2013-05-29 16:11:21]

I've made a final change as was agreed by jam@ and tommi@. The log messages now
go via //content.

John and Tommi: can you please have a look so that I didn't miss anything.

Avi: please check the new/changed
content/renderer/media/webrtc_logging_initializer_impl.h/cc
and
content/public/renderer/media/webrtc_logging_initializer.h

[Avi (use Gerrit), 2013-05-29 16:12:47]

SLGTM

[jam, 2013-05-29 16:25:48]

https://codereview.chromium.org/15741003/diff/39002/content/public/renderer/m...
File content/public/renderer/media/webrtc_log_message_delegate.h (right):

https://codereview.chromium.org/15741003/diff/39002/content/public/renderer/m...
content/public/renderer/media/webrtc_log_message_delegate.h:14: // purpose is to
forward mainly libjingle log messages to Chrome (besides to the
nit: don't mention chrome in content

https://codereview.chromium.org/15741003/diff/39002/content/public/renderer/m...
content/public/renderer/media/webrtc_log_message_delegate.h:19: const
std::string& app_url) = 0;
what are these two parameters?

nit: also document these two methods

[Henrik Grunell, 2013-05-29 16:56:58]

https://chromiumcodereview.appspot.com/15741003/diff/39002/content/public/ren...
File content/public/renderer/media/webrtc_log_message_delegate.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/39002/content/public/ren...
content/public/renderer/media/webrtc_log_message_delegate.h:14: // purpose is to
forward mainly libjingle log messages to Chrome (besides to the
On 2013/05/29 16:25:48, jam wrote:
> nit: don't mention chrome in content

Changed to "embedder".

https://chromiumcodereview.appspot.com/15741003/diff/39002/content/public/ren...
content/public/renderer/media/webrtc_log_message_delegate.h:19: const
std::string& app_url) = 0;
On 2013/05/29 16:25:48, jam wrote:
> what are these two parameters?

The app session ID comes from the constraint value and is set by JS. The url is
the document url that triggered to start logging. (See
media_stream_dependency_factory.cc.)
 
> nit: also document these two methods

Done.

[jam, 2013-05-29 17:56:19]

(just looked through content, this time more thoroughly)

https://codereview.chromium.org/15741003/diff/81001/content/public/renderer/m...
File content/public/renderer/media/webrtc_log_message_delegate.h (right):

https://codereview.chromium.org/15741003/diff/81001/content/public/renderer/m...
content/public/renderer/media/webrtc_log_message_delegate.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
do not add a media subdir under content/public/renderer. we put all the
interfaces in content/public/renderer (with the exception for platform specific
ones like android)

https://codereview.chromium.org/15741003/diff/81001/content/public/renderer/m...
File content/public/renderer/media/webrtc_logging_initializer.h (right):

https://codereview.chromium.org/15741003/diff/81001/content/public/renderer/m...
content/public/renderer/media/webrtc_logging_initializer.h:1: // Copyright 2013
The Chromium Authors. All rights reserved.
ditto

https://codereview.chromium.org/15741003/diff/81001/content/public/renderer/m...
content/public/renderer/media/webrtc_logging_initializer.h:15: CONTENT_EXPORT
void InitWebRtcLoggingDelegate(
actually, why not just move this one method to
content/public/renderer/webrtc_log_message_delegate.h (after the interface)

https://codereview.chromium.org/15741003/diff/81001/content/renderer/media/we...
File content/renderer/media/webrtc_logging_initializer_impl.h (right):

https://codereview.chromium.org/15741003/diff/81001/content/renderer/media/we...
content/renderer/media/webrtc_logging_initializer_impl.h:1: // Copyright 2013
The Chromium Authors. All rights reserved.
it's odd that this file has _impl in its name although it doesn't implement any
interfaces.

per my other comment, if you get rid of webrtc_logging_initializer.h in
content/public/renderer, then remove the "_impl" from these two files in
content/renderer/media

https://codereview.chromium.org/15741003/diff/81001/content/renderer/media/we...
content/renderer/media/webrtc_logging_initializer_impl.h:17: CONTENT_EXPORT void
InitWebRtcLogging(const std::string& app_session_id,
you sure these two methods need to be exported?

[Henrik Grunell, 2013-05-29 18:25:51]

https://chromiumcodereview.appspot.com/15741003/diff/81001/content/public/ren...
File content/public/renderer/media/webrtc_log_message_delegate.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/81001/content/public/ren...
content/public/renderer/media/webrtc_log_message_delegate.h:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
On 2013/05/29 17:56:19, jam wrote:
> do not add a media subdir under content/public/renderer. we put all the
> interfaces in content/public/renderer (with the exception for platform
specific
> ones like android)

Done.

https://chromiumcodereview.appspot.com/15741003/diff/81001/content/public/ren...
File content/public/renderer/media/webrtc_logging_initializer.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/81001/content/public/ren...
content/public/renderer/media/webrtc_logging_initializer.h:15: CONTENT_EXPORT
void InitWebRtcLoggingDelegate(
On 2013/05/29 17:56:19, jam wrote:
> actually, why not just move this one method to
> content/public/renderer/webrtc_log_message_delegate.h (after the interface)

Done.

https://chromiumcodereview.appspot.com/15741003/diff/81001/content/renderer/m...
File content/renderer/media/webrtc_logging_initializer_impl.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/81001/content/renderer/m...
content/renderer/media/webrtc_logging_initializer_impl.h:1: // Copyright 2013
The Chromium Authors. All rights reserved.
On 2013/05/29 17:56:19, jam wrote:
> it's odd that this file has _impl in its name although it doesn't implement
any
> interfaces.
> 
> per my other comment, if you get rid of webrtc_logging_initializer.h in
> content/public/renderer, then remove the "_impl" from these two files in
> content/renderer/media

Done.

https://chromiumcodereview.appspot.com/15741003/diff/81001/content/renderer/m...
content/renderer/media/webrtc_logging_initializer_impl.h:17: CONTENT_EXPORT void
InitWebRtcLogging(const std::string& app_session_id,
On 2013/05/29 17:56:19, jam wrote:
> you sure these two methods need to be exported?

Right, they don't need to.

[jam, 2013-05-30 15:01:17]

lgtm, thanks for doing this

https://chromiumcodereview.appspot.com/15741003/diff/88001/chrome/browser/med...
File chrome/browser/media/webrtc_logging_handler_host.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/88001/chrome/browser/med...
chrome/browser/media/webrtc_logging_handler_host.h:12: namespace chrome {
nit: here and in other files you moved to chrome: in general code in chrome
doesn't have to be in a namespace. i'd take this out

https://chromiumcodereview.appspot.com/15741003/diff/88001/chrome/renderer/me...
File chrome/renderer/media/webrtc_logging_handler_impl.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/88001/chrome/renderer/me...
chrome/renderer/media/webrtc_logging_handler_impl.h:27: class
WebRtcLoggingHandlerImpl
nit: chrome implementations of content interface Foo are usually named
ChromeFoo, so I would name this ChromeWebRtcLoggingHandler

https://chromiumcodereview.appspot.com/15741003/diff/88001/content/renderer/m...
File content/renderer/media/webrtc_logging_initializer.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/88001/content/renderer/m...
content/renderer/media/webrtc_logging_initializer.h:5: #ifndef
CONTENT_RENDERER_WEBRTC_LOGGING_INITIALIZER_IMPL_H_
nit: update

https://chromiumcodereview.appspot.com/15741003/diff/88001/content/renderer/m...
content/renderer/media/webrtc_logging_initializer.h:10: #include
"content/common/content_export.h"
nit: not necessary

[Henrik Grunell, 2013-05-31 19:26:38]

https://chromiumcodereview.appspot.com/15741003/diff/88001/chrome/browser/med...
File chrome/browser/media/webrtc_logging_handler_host.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/88001/chrome/browser/med...
chrome/browser/media/webrtc_logging_handler_host.h:12: namespace chrome {
On 2013/05/30 15:01:17, jam wrote:
> nit: here and in other files you moved to chrome: in general code in chrome
> doesn't have to be in a namespace. i'd take this out

Done.

https://chromiumcodereview.appspot.com/15741003/diff/88001/chrome/renderer/me...
File chrome/renderer/media/webrtc_logging_handler_impl.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/88001/chrome/renderer/me...
chrome/renderer/media/webrtc_logging_handler_impl.h:27: class
WebRtcLoggingHandlerImpl
On 2013/05/30 15:01:17, jam wrote:
> nit: chrome implementations of content interface Foo are usually named
> ChromeFoo, so I would name this ChromeWebRtcLoggingHandler

So, wouldn't this class be called ChromeWebRtcLogMessageDelegate then to follow
the naming convention strictly? I'll land this CL and change the name separately
afterwards based on your response.

https://chromiumcodereview.appspot.com/15741003/diff/88001/content/renderer/m...
File content/renderer/media/webrtc_logging_initializer.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/88001/content/renderer/m...
content/renderer/media/webrtc_logging_initializer.h:5: #ifndef
CONTENT_RENDERER_WEBRTC_LOGGING_INITIALIZER_IMPL_H_
On 2013/05/30 15:01:17, jam wrote:
> nit: update

Done.

https://chromiumcodereview.appspot.com/15741003/diff/88001/content/renderer/m...
content/renderer/media/webrtc_logging_initializer.h:10: #include
"content/common/content_export.h"
On 2013/05/30 15:01:17, jam wrote:
> nit: not necessary

Done.

[jam, 2013-05-31 19:38:10]

https://chromiumcodereview.appspot.com/15741003/diff/88001/chrome/renderer/me...
File chrome/renderer/media/webrtc_logging_handler_impl.h (right):

https://chromiumcodereview.appspot.com/15741003/diff/88001/chrome/renderer/me...
chrome/renderer/media/webrtc_logging_handler_impl.h:27: class
WebRtcLoggingHandlerImpl
On 2013/05/31 19:26:38, Henrik Grunell wrote:
> On 2013/05/30 15:01:17, jam wrote:
> > nit: chrome implementations of content interface Foo are usually named
> > ChromeFoo, so I would name this ChromeWebRtcLoggingHandler
> 
> So, wouldn't this class be called ChromeWebRtcLogMessageDelegate then to
follow
> the naming convention strictly? I'll land this CL and change the name
separately
> afterwards based on your response.

oops, sorry yes that would be ChromeWebRtcLogMessageDelegate

[commit-bot: I haz the power, 2013-05-31 19:57:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grunell@chromium.org/15741003/100001

[commit-bot: I haz the power, 2013-05-31 23:58:33]

Change committed as 203516