Hook up WebRTC logging extension API to the underlying functionality.

Hook up WebRTC logging extension API to the underlying functionality.

The API skeleton CL landed previously: https://chromiumcodereview.appspot.com/23885002/

In the CL:
- Hook up the API to the logging functionality.
- Changes to the logging functionality to allow for new API features.
- Make RenderProcessHost a SupportsUserData, to be able to associate a logging handler host with it.
- Made a fix in base/supports_user_data.h.
- Removal of the old way of enabling logging via the JS PeerConnection constructor.
- Refactorings due to the new API features and removals.

TBR=jochen@chromium.org (for chrome/common/media/webrtc_logging_messages.h)
BUG=298158
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226805

[Henrik Grunell, 2013-09-27 14:01:50]

Hi, please review this. Idea of dividing (based on owners), feel free to review
as much as you like.

mark@: base/*

jln@: chrome/common/media/webrtc_logging_messages.h

joi@: content/public/*

finnur@: 
chrome/browser/extensions/*
chrome/common/extensions/*

xians@:
chrome/browser/media/*
chrome/renderer/media/*
content/renderer/media/*

[Finnur, 2013-09-27 14:10:06]

From an OWNERS perspective the extension stuff LGTM, but you should probably get
someone with webrtc experience to look at at least the two files in
chrome/browser/extensions/.

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h
(right):

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h:26:
void SetMetaDataCallback(bool success, std::string error_message);
nit: const std::string&  ?

Same below.

https://codereview.chromium.org/23691066/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/23691066/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/_permission_features.json:765: "channel": "stable",
The change looks fine, but I think scheib should probably give the green light
on this going to stable, as he has followed this API through the approval
process.

[Henrik Grunell, 2013-09-27 14:35:17]

On 2013/09/27 14:10:06, Finnur wrote:
> From an OWNERS perspective the extension stuff LGTM, but you should probably
get
> someone with webrtc experience to look at at least the two files in
> chrome/browser/extensions/.

OK, I added vrk@. Victoria, Can you please review chrome/browser/extensions/*
(and anything else if you like).

Joi: I believe all the files in */media/* in this CL except
media_stream_dependency_factory.h/cc were outside of media before and you have
reviewed most of it. Maybe you can work something out with Shijing regarding
reviewing that, since it's new to him. Still need an owner.

[Jói, 2013-09-27 14:39:52]

//content/public LGTM.

I did a quick pass on everything else in the change, see comments below. The
folks you nominated for specific parts will hopefully do a more thorough pass.

Cheers,
Jói

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc
(right):

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:48:
meta_data, callback));
Just noting it here, probably not worth optimizing if the meta_data map is
generally fairly small: This will cause the entire map to be copied into the
posted task. If it's large you could use a linked_ptr<> of the map type instead.

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/media/webrt...
File chrome/browser/media/webrtc_log_uploader.h (right):

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/media/webrt...
chrome/browser/media/webrtc_log_uploader.h:55: // uploaded. Decreases log count.
May only be called if permission to
"to to" (one at the end of this line, one at start of next)

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/media/webrt...
chrome/browser/media/webrtc_log_uploader.h:62: // Decreases log count. May only
be called if permission to
"to to"

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/media/webrt...
File chrome/browser/media/webrtc_logging_handler_host.cc (right):

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/media/webrt...
chrome/browser/media/webrtc_logging_handler_host.cc:349:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Might want to DCHECK on logging_state_ also to help future maintainers.

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/media/webrt...
File chrome/browser/media/webrtc_logging_handler_host.h (right):

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/media/webrt...
chrome/browser/media/webrtc_logging_handler_host.h:65: // States used for
protecting from function calls made at non-allowed points
I <3 DFMSs!

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/media/webrt...
chrome/browser/media/webrtc_logging_handler_host.h:67: // Transitions:
OpenLog(): CLOSED -> STARTING.
Missing transition: SetMetaData(): CLOSED -> CLOSED.

https://codereview.chromium.org/23691066/diff/3001/content/renderer/media/web...
File content/renderer/media/webrtc_logging_initializer.cc (right):

https://codereview.chromium.org/23691066/diff/3001/content/renderer/media/web...
content/renderer/media/webrtc_logging_initializer.cc:24: void
InitWebRtcLogging() {
Did you mean to completely remove the declaration of this function from the
header? Or did you mean for this to be an implementation of
WebRtcLogMessageDelegate::InitWebRtcLogging?

[Jói, 2013-09-27 14:54:06]

> Joi: I believe all the files in */media/* in this CL except
> media_stream_dependency_factory.h/cc were outside of media before and you
> have
> reviewed most of it. Maybe you can work something out with Shijing regarding
> reviewing that, since it's new to him. Still need an owner.

Yes, I'm familiar with them. I've done another more detailed pass on
the */media/* files. No new comments beyond those I already sent.

Cheers,
Jói


On Fri, Sep 27, 2013 at 2:35 PM,  <grunell@chromium.org> wrote:
> On 2013/09/27 14:10:06, Finnur wrote:
>>
>>  From an OWNERS perspective the extension stuff LGTM, but you should
>> probably
>
> get
>>
>> someone with webrtc experience to look at at least the two files in
>> chrome/browser/extensions/.
>
>
> OK, I added vrk@. Victoria, Can you please review
> chrome/browser/extensions/*
> (and anything else if you like).
>
> Joi: I believe all the files in */media/* in this CL except
> media_stream_dependency_factory.h/cc were outside of media before and you
> have
> reviewed most of it. Maybe you can work something out with Shijing regarding
> reviewing that, since it's new to him. Still need an owner.
>
> https://codereview.chromium.org/23691066/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Mentovai, 2013-09-27 15:19:14]

I’m out until Tuesday, for faster service, you can pick an alternate reviewer
for the base portion.

[Henrik Grunell, 2013-09-30 08:34:56]

On 2013/09/27 15:19:14, Mark Mentovai wrote:
> I’m out until Tuesday, for faster service, you can pick an alternate reviewer
> for the base portion.

willchan@: please review base/*

[willchan no longer on Chromium, 2013-09-30 16:15:12]

lgtm

[jln (very slow on Chromium), 2013-09-30 23:26:12]

chrome/common/media/webrtc_logging_messages.h lgtm, under the assumption that
there isn't really anything new. Please let me know if I got that wrong.

[Henrik Grunell, 2013-10-01 06:29:39]

On 2013/09/30 23:26:12, jln wrote:
> chrome/common/media/webrtc_logging_messages.h lgtm, under the assumption that
> there isn't really anything new. Please let me know if I got that wrong.

Well, depends on what you mean with new? The StopLogging and LoggingStopped
messages are new.

[no longer working on chromium, 2013-10-01 08:41:41]

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc
(right):

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:38:
std::vector<linked_ptr<api::webrtc_logging_private::MetaDataEntry> >::iterator
Put it in the scope of for loop, and use const_iterator

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/media/webrt...
File chrome/browser/media/webrtc_log_uploader.h (right):

https://codereview.chromium.org/23691066/diff/3001/chrome/browser/media/webrt...
chrome/browser/media/webrtc_log_uploader.h:8: #include <string>
include what you are using:
include <map>

https://codereview.chromium.org/23691066/diff/3001/content/renderer/media/med...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/23691066/diff/3001/content/renderer/media/med...
content/renderer/media/media_stream_dependency_factory.cc:24: #include
"content/renderer/media/webrtc_logging_initializer.h"
remove if you don't need this

https://codereview.chromium.org/23691066/diff/3001/content/renderer/media/web...
File content/renderer/media/webrtc_logging_initializer.h (left):

https://codereview.chromium.org/23691066/diff/3001/content/renderer/media/web...
content/renderer/media/webrtc_logging_initializer.h:19: void
WebRtcLogMessage(const std::string& message);
It looks odd to have the initializer being used as a dispatcher to forward the
error message, why this file is needed? can you merge the method to other file,
like chrome_webrtc_log_message_delegate.cc?

[Henrik Grunell, 2013-10-02 12:47:18]

* Code review fixes. See diff patch 2->3.
* Removed app_url; the url can be set in the meta data instead. See diff patch
4->5.

(Patch 3 and 4 are equal.)

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/exte...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc
(right):

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:38:
std::vector<linked_ptr<api::webrtc_logging_private::MetaDataEntry> >::iterator
On 2013/10/01 08:41:42, xians1 wrote:
> Put it in the scope of for loop, and use const_iterator

Done.

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc:48:
meta_data, callback));
On 2013/09/27 14:39:53, Jói wrote:
> Just noting it here, probably not worth optimizing if the meta_data map is
> generally fairly small: This will cause the entire map to be copied into the
> posted task. If it's large you could use a linked_ptr<> of the map type
instead.

Yes, it's expected to be in the order of 5 entries. I'll leave it unless you or
anyone else objects.

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/exte...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h
(right):

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h:26:
void SetMetaDataCallback(bool success, std::string error_message);
On 2013/09/27 14:10:06, Finnur wrote:
> nit: const std::string&  ?
> 
> Same below.

Absolutely, done.

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/medi...
File chrome/browser/media/webrtc_log_uploader.h (right):

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/medi...
chrome/browser/media/webrtc_log_uploader.h:8: #include <string>
On 2013/10/01 08:41:42, xians1 wrote:
> include what you are using:
> include <map>

Done.

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/medi...
chrome/browser/media/webrtc_log_uploader.h:55: // uploaded. Decreases log count.
May only be called if permission to
On 2013/09/27 14:39:53, Jói wrote:
> "to to" (one at the end of this line, one at start of next)

Done.

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/medi...
chrome/browser/media/webrtc_log_uploader.h:62: // Decreases log count. May only
be called if permission to
On 2013/09/27 14:39:53, Jói wrote:
> "to to"

Done.

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/medi...
File chrome/browser/media/webrtc_logging_handler_host.cc (right):

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/medi...
chrome/browser/media/webrtc_logging_handler_host.cc:349:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2013/09/27 14:39:53, Jói wrote:
> Might want to DCHECK on logging_state_ also to help future maintainers.

Done.

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/medi...
File chrome/browser/media/webrtc_logging_handler_host.h (right):

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/medi...
chrome/browser/media/webrtc_logging_handler_host.h:65: // States used for
protecting from function calls made at non-allowed points
On 2013/09/27 14:39:53, Jói wrote:
> I <3 DFMSs!

Happy to hear. :)

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/browser/medi...
chrome/browser/media/webrtc_logging_handler_host.h:67: // Transitions:
OpenLog(): CLOSED -> STARTING.
On 2013/09/27 14:39:53, Jói wrote:
> Missing transition: SetMetaData(): CLOSED -> CLOSED.

Done. Also updated the function and state names in the comment.

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/common/exten...
File chrome/common/extensions/api/_permission_features.json (right):

https://chromiumcodereview.appspot.com/23691066/diff/3001/chrome/common/exten...
chrome/common/extensions/api/_permission_features.json:765: "channel": "stable",
On 2013/09/27 14:10:06, Finnur wrote:
> The change looks fine, but I think scheib should probably give the green light
> on this going to stable, as he has followed this API through the approval
> process.

I've asked miket@ for his blessing. Won't commit without it.

https://chromiumcodereview.appspot.com/23691066/diff/3001/content/renderer/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://chromiumcodereview.appspot.com/23691066/diff/3001/content/renderer/me...
content/renderer/media/media_stream_dependency_factory.cc:24: #include
"content/renderer/media/webrtc_logging_initializer.h"
On 2013/10/01 08:41:42, xians1 wrote:
> remove if you don't need this

Done.

https://chromiumcodereview.appspot.com/23691066/diff/3001/content/renderer/me...
File content/renderer/media/webrtc_logging_initializer.cc (right):

https://chromiumcodereview.appspot.com/23691066/diff/3001/content/renderer/me...
content/renderer/media/webrtc_logging_initializer.cc:24: void
InitWebRtcLogging() {
On 2013/09/27 14:39:53, Jói wrote:
> Did you mean to completely remove the declaration of this function from the
> header? Or did you mean for this to be an implementation of
> WebRtcLogMessageDelegate::InitWebRtcLogging?

The "old" InitWebRtcLogging has been removed. It was called from
MediaStreamFactory, triggered from javascript.

The "new" InitWebRtcLogging is called from the delegate, triggered from
extension.

(It's not a member of WebRtcLogMessageDelegate.)

https://chromiumcodereview.appspot.com/23691066/diff/3001/content/renderer/me...
File content/renderer/media/webrtc_logging_initializer.h (left):

https://chromiumcodereview.appspot.com/23691066/diff/3001/content/renderer/me...
content/renderer/media/webrtc_logging_initializer.h:19: void
WebRtcLogMessage(const std::string& message);
On 2013/10/01 08:41:42, xians1 wrote:
> It looks odd to have the initializer being used as a dispatcher to forward the
> error message, why this file is needed? can you merge the method to other
file,
> like chrome_webrtc_log_message_delegate.cc?

There was quite a long discussion about this with jam@, joi@ and tommi@. I can
give you details off-review if you like.

[no longer working on chromium, 2013-10-02 12:55:10]

lgtm on media code.

https://chromiumcodereview.appspot.com/23691066/diff/3001/content/renderer/me...
File content/renderer/media/webrtc_logging_initializer.h (left):

https://chromiumcodereview.appspot.com/23691066/diff/3001/content/renderer/me...
content/renderer/media/webrtc_logging_initializer.h:19: void
WebRtcLogMessage(const std::string& message);
On 2013/10/02 12:47:18, Henrik Grunell wrote:
> On 2013/10/01 08:41:42, xians1 wrote:
> > It looks odd to have the initializer being used as a dispatcher to forward
the
> > error message, why this file is needed? can you merge the method to other
> file,
> > like chrome_webrtc_log_message_delegate.cc?
> 
> There was quite a long discussion about this with jam@, joi@ and tommi@. I can
> give you details off-review if you like.

All right, just keep what it is if that is some kind of conclusion from that
discussion.

[vrk (LEFT CHROMIUM), 2013-10-03 06:53:54]

lgtm

[commit-bot: I haz the power, 2013-10-03 09:16:25]

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

[commit-bot: I haz the power, 2013-10-03 09:29:51]

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

[commit-bot: I haz the power, 2013-10-03 09:47:53]

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

[Henrik Grunell, 2013-10-03 10:01:07]

TBR'ing jochen@ as owner for chrome/common/media/webrtc_logging_messages.h. It
has been reviewed by others already, including security.

[commit-bot: I haz the power, 2013-10-03 10:16:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Henrik Grunell, 2013-10-03 11:14:35]

Updated a unit test. (Patch set 8)

Shijing, please take a look.

[no longer working on chromium, 2013-10-03 11:21:15]

On 2013/10/03 11:14:35, Henrik Grunell wrote:
> Updated a unit test. (Patch set 8)
> 
> Shijing, please take a look.

still lgtm

[commit-bot: I haz the power, 2013-10-03 11:27:06]

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

[commit-bot: I haz the power, 2013-10-03 13:12:24]

Retried try job too often on win_rel for step(s) content_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-03 16:03:30]

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

[commit-bot: I haz the power, 2013-10-03 18:44:08]

Change committed as 226805