Adds StartRtpDump and StopRtpDump methods to webrtcLoggingPrivate API.

Adds StartRtpDump and StopRtpDump methods to webrtcLoggingPrivate API.
It's only a stub for now.

Design doc: https://docs.google.com/a/google.com/document/d/1qnzq1JABVObATtwGcstmmglXbOtaNZawxEjtNQCWwq8/edit?disco=AAAAAJYJFCI#heading=h.hom4iccr2c3o

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

[jiayl, 2014-04-30 22:27:39]

PTAL.

[jiayl, 2014-04-30 22:29:33]

vrk: webrtc_logging_handler_host.*
grunell: webrtc_logging_private_api.*
miket: .idl
asvitkine: extension_function_histogram_value

[Alexei Svitkine (slow), 2014-05-01 15:27:27]

Please also update histograms.xml with the new enum values.

[jiayl, 2014-05-01 16:13:30]

On 2014/05/01 15:27:27, Alexei Svitkine wrote:
> Please also update histograms.xml with the new enum values.

Done.

[Alexei Svitkine (slow), 2014-05-01 16:35:06]

histograms/enums lgtm

[vrk (LEFT CHROMIUM), 2014-05-02 18:13:48]

lgtm with nits

https://codereview.chromium.org/267623003/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api_stub.cc
(right):

https://codereview.chromium.org/267623003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api_stub.cc:99:
WebrtcLoggingPrivateStartRtpDumpFunction() {
nit: Here and elsewhere, all empty function bodies should be {} (one line, not
two).

https://codereview.chromium.org/267623003/diff/20001/chrome/browser/media/web...
File chrome/browser/media/webrtc_logging_handler_host.h (right):

https://codereview.chromium.org/267623003/diff/20001/chrome/browser/media/web...
chrome/browser/media/webrtc_logging_handler_host.h:77: // on the IO thread.
nit: expand comment to explain the parameters.

https://codereview.chromium.org/267623003/diff/20001/chrome/browser/media/web...
chrome/browser/media/webrtc_logging_handler_host.h:83: // on the IO thread.
nit: expand comment to explain the parameters.

https://codereview.chromium.org/267623003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/webrtc_logging_private.idl (right):

https://codereview.chromium.org/267623003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webrtc_logging_private.idl:60: // Uploads the log
and the RTP dumps. Logging must be stopped before this
Nit: maybe clarify, "the RTP dumps, if they exist."

[jiayl, 2014-05-02 21:22:43]

https://codereview.chromium.org/267623003/diff/20001/chrome/browser/extension...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api_stub.cc
(right):

https://codereview.chromium.org/267623003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api_stub.cc:99:
WebrtcLoggingPrivateStartRtpDumpFunction() {
On 2014/05/02 18:13:48, Victoria Kirst wrote:
> nit: Here and elsewhere, all empty function bodies should be {} (one line, not
> two).

Done.

https://codereview.chromium.org/267623003/diff/20001/chrome/browser/media/web...
File chrome/browser/media/webrtc_logging_handler_host.h (right):

https://codereview.chromium.org/267623003/diff/20001/chrome/browser/media/web...
chrome/browser/media/webrtc_logging_handler_host.h:77: // on the IO thread.
On 2014/05/02 18:13:48, Victoria Kirst wrote:
> nit: expand comment to explain the parameters.

Done.

https://codereview.chromium.org/267623003/diff/20001/chrome/browser/media/web...
chrome/browser/media/webrtc_logging_handler_host.h:83: // on the IO thread.
On 2014/05/02 18:13:48, Victoria Kirst wrote:
> nit: expand comment to explain the parameters.

Done.

https://codereview.chromium.org/267623003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/webrtc_logging_private.idl (right):

https://codereview.chromium.org/267623003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webrtc_logging_private.idl:60: // Uploads the log
and the RTP dumps. Logging must be stopped before this
On 2014/05/02 18:13:48, Victoria Kirst wrote:
> Nit: maybe clarify, "the RTP dumps, if they exist."

Done.

[jiayl, 2014-05-02 21:23:03]

Ping Mike and Henrik.

Will you take a look? Thanks!

[Henrik Grunell, 2014-05-05 14:15:09]

Sorry for the delay, public holiday + vacation end of last week.

https://codereview.chromium.org/267623003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/webrtc_logging_private.idl (right):

https://codereview.chromium.org/267623003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/webrtc_logging_private.idl:41: static void
start(long tabId,
I'd like "start, "stop" and "discard" to be renamed to say "startDiagnosticLog"
etc. or such. To be clear what they do. It requires updates in the users of the
API, I'm fine with doing such API change in a follow-up CL.

https://codereview.chromium.org/267623003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/webrtc_logging_private.idl:84: boolean incoming,
Is |incoming| and |outgoing| necessary? Maybe simply stop whatever has started?

[jiayl, 2014-05-05 16:26:54]

https://codereview.chromium.org/267623003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/webrtc_logging_private.idl (right):

https://codereview.chromium.org/267623003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/webrtc_logging_private.idl:41: static void
start(long tabId,
On 2014/05/05 14:15:09, Henrik Grunell wrote:
> I'd like "start, "stop" and "discard" to be renamed to say
"startDiagnosticLog"
> etc. or such. To be clear what they do. It requires updates in the users of
the
> API, I'm fine with doing such API change in a follow-up CL.

Ack. https://code.google.com/p/chromium/issues/detail?id=370040

https://codereview.chromium.org/267623003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/webrtc_logging_private.idl:84: boolean incoming,
On 2014/05/05 14:15:09, Henrik Grunell wrote:
> Is |incoming| and |outgoing| necessary? Maybe simply stop whatever has
started?

The existing UI for the GTP allows the user to check/uncheck incoming/outgoing
dump separately. So I think we might need this flexibility.

[miket_OOO, 2014-05-05 17:58:54]

lgtm

https://codereview.chromium.org/267623003/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api_stub.cc
(right):

https://codereview.chromium.org/267623003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api_stub.cc:107:
SetError(kErrorNotSupported);
We sometimes DCHECK_CURRENTLY_ON(BrowserThread::IO) if you think there's any
chance of confusion. Up to you.

[Henrik Grunell, 2014-05-06 08:04:10]

lgtm

[jiayl, 2014-05-06 16:17:57]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-05-06 16:18:42]

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

[commit-bot: I haz the power, 2014-05-06 16:48:59]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
  android_clang_dbg on tryserver.chromium
  android_dbg on tryserver.chromium
  chromium_presubmit on tryserver.chromium
  ios_dbg_simulator on tryserver.chromium
  ios_rel_device on tryserver.chromium
  ios_rel_device_ninja on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium
  mac_chromium_compile_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium
  win_chromium_compile_dbg on tryserver.chromium
  win_chromium_rel on tryserver.chromium
  win_chromium_x64_rel on tryserver.chromium

[jiayl, 2014-05-06 17:06:30]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-05-06 17:06:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiayl@chromium.org/267623003/60001

[jiayl, 2014-05-06 17:51:39]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-05-06 17:53:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiayl@chromium.org/267623003/80001

[commit-bot: I haz the power, 2014-05-07 11:43:53]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 13:52:43]

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

[commit-bot: I haz the power, 2014-05-07 13:52:43]

Failed to apply patch for
extensions/browser/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file extensions/browser/extension_function_histogram_value.h
  Hunk #1 FAILED at 812.
  1 out of 1 hunk FAILED -- saving rejects to file
extensions/browser/extension_function_histogram_value.h.rej

Patch:       extensions/browser/extension_function_histogram_value.h
Index: extensions/browser/extension_function_histogram_value.h
diff --git a/extensions/browser/extension_function_histogram_value.h
b/extensions/browser/extension_function_histogram_value.h
index
8cac2aa9faaa057d249926274b6c367284b49c10..39903925428c43bb01e4eb99e93ab805f12d5ee8
100644
--- a/extensions/browser/extension_function_histogram_value.h
+++ b/extensions/browser/extension_function_histogram_value.h
@@ -812,6 +812,8 @@ enum HistogramValue {
   FILESYSTEMPROVIDERINTERNAL_GETMETADATAREQUESTEDERROR,
   BROWSER_OPENTAB,
   MANAGEMENT_CREATEAPPSHORTCUT,
+  WEBRTCLOGGINGPRIVATE_STARTRTPDUMP,
+  WEBRTCLOGGINGPRIVATE_STOPRTPDUMP,
   // Last entry: Add new entries above and ensure to update
   // tools/metrics/histograms/histograms/histograms.xml.
   ENUM_BOUNDARY

[jiayl, 2014-05-07 15:59:26]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-05-07 16:00:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiayl@chromium.org/267623003/80001

[commit-bot: I haz the power, 2014-05-07 16:03:03]

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

[commit-bot: I haz the power, 2014-05-07 16:03:04]

Failed to apply patch for
extensions/browser/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file extensions/browser/extension_function_histogram_value.h
  Hunk #1 FAILED at 812.
  1 out of 1 hunk FAILED -- saving rejects to file
extensions/browser/extension_function_histogram_value.h.rej

Patch:       extensions/browser/extension_function_histogram_value.h
Index: extensions/browser/extension_function_histogram_value.h
diff --git a/extensions/browser/extension_function_histogram_value.h
b/extensions/browser/extension_function_histogram_value.h
index
8cac2aa9faaa057d249926274b6c367284b49c10..39903925428c43bb01e4eb99e93ab805f12d5ee8
100644
--- a/extensions/browser/extension_function_histogram_value.h
+++ b/extensions/browser/extension_function_histogram_value.h
@@ -812,6 +812,8 @@ enum HistogramValue {
   FILESYSTEMPROVIDERINTERNAL_GETMETADATAREQUESTEDERROR,
   BROWSER_OPENTAB,
   MANAGEMENT_CREATEAPPSHORTCUT,
+  WEBRTCLOGGINGPRIVATE_STARTRTPDUMP,
+  WEBRTCLOGGINGPRIVATE_STOPRTPDUMP,
   // Last entry: Add new entries above and ensure to update
   // tools/metrics/histograms/histograms/histograms.xml.
   ENUM_BOUNDARY

[jiayl, 2014-05-07 16:05:44]

The CQ bit was checked by jiayl@chromium.org

[commit-bot: I haz the power, 2014-05-07 16:09:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiayl@chromium.org/267623003/90001

[commit-bot: I haz the power, 2014-05-07 23:05:16]

Change committed as 268988