Wired chrome.rtcPrivate API.

Wired chrome.rtcPrivate API.

BUG=141764
TEST=RtcPrivateApiTest.*

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

[Mihai Parparita -not on Chrome, 2012-09-05 04:42:47]

(drive-by)

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/chrome_brows...
File chrome/chrome_browser_extensions.gypi (right):

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/chrome_brows...
chrome/chrome_browser_extensions.gypi:602: ['exclude',
'browser/extensions/api/rtc_private/rtc_private_api.cc'],
These files seem to be missing from the CL.

Also, ChromeOS-only APIs can go in chrome/browser/chromeos/extensions.

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/common/chrom...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/common/chrom...
chrome/common/chrome_switches.cc:494: const char kTestRtcExtensionId[]          
 = "test-rtc-extension";
Do you need an additional flag for this? kWhitelistedExtensionID can already be
used to add an additional extension ID (for all whitelisted APIs).

[Daniel Erat, 2012-09-05 14:05:55]

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/chrome_tests...
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/chrome_tests...
chrome/chrome_tests.gypi:2825:
'browser/extensions/api/rtc_private/rtc_private_apitest.cc',
nit: alphabetize

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/common/exten...
File chrome/common/extensions/api/api.gyp (right):

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/common/exten...
chrome/common/extensions/api/api.gyp:61: 'rtc_private.idl',
nit: alphabetize

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/common/exten...
File chrome/common/extensions/api/rtc_private.idl (right):

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/common/exten...
chrome/common/extensions/api/rtc_private.idl:10: // Launch action.
nit: fix this comment

[zel, 2012-09-05 18:50:33]

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/chrome_brows...
File chrome/chrome_browser_extensions.gypi (right):

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/chrome_brows...
chrome/chrome_browser_extensions.gypi:602: ['exclude',
'browser/extensions/api/rtc_private/rtc_private_api.cc'],
On 2012/09/05 04:42:47, Mihai Parparita wrote:
> These files seem to be missing from the CL.
> 
> Also, ChromeOS-only APIs can go in chrome/browser/chromeos/extensions.

Whoops! Added missing files, PTAL.

About the chrome/browser/chromeos/extensions - that's where I originally had it,
but the code generated form <foo>.idl file expected API to live in
browser/extensions/api/<foo>. Mihai, is there any way to hint the idl compiler
that api implementation is somewhere else?

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/chrome_tests...
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/chrome_tests...
chrome/chrome_tests.gypi:2825:
'browser/extensions/api/rtc_private/rtc_private_apitest.cc',
On 2012/09/05 14:05:55, Daniel Erat wrote:
> nit: alphabetize

Done.

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/common/chrom...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/common/chrom...
chrome/common/chrome_switches.cc:494: const char kTestRtcExtensionId[]          
 = "test-rtc-extension";
On 2012/09/05 04:42:47, Mihai Parparita wrote:
> Do you need an additional flag for this? kWhitelistedExtensionID can already
be
> used to add an additional extension ID (for all whitelisted APIs).

I used to send this event only to that particular extension. Since no others
than whitelisted can listen it, you are right that I can get rid of this now.

Done.

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/common/exten...
File chrome/common/extensions/api/api.gyp (right):

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/common/exten...
chrome/common/extensions/api/api.gyp:61: 'rtc_private.idl',
On 2012/09/05 14:05:55, Daniel Erat wrote:
> nit: alphabetize

Done.

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/common/exten...
File chrome/common/extensions/api/rtc_private.idl (right):

https://chromiumcodereview.appspot.com/10919086/diff/7001/chrome/common/exten...
chrome/common/extensions/api/rtc_private.idl:10: // Launch action.
On 2012/09/05 14:05:55, Daniel Erat wrote:
> nit: fix this comment

Done.

[Daniel Erat, 2012-09-05 19:28:32]

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.cc (right):

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:11: #include
"base/stringprintf.h"
nit: alphabetize

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:73: break;
nit: add a default case with NOTREACHED() << "Unknown action " << action;

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:80:
scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
nit: any reason that you actually need a scoped_ptr here instead of just
allocating this on the stack?

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:88: for (i = 0; i <
contact->phone_numbers_size(); i++) {
nit: move 'i' declaration into the loop condition

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:95: for (i = 0; i <
contact->email_addresses_size(); i++) {
we talked earlier about sorting phone numbers and email addresses so the entry
with its 'primary' field set to true will come first.  how about doing something
like this instead:

  for (...) {
    const Contact_EmailAddress& email_address = ...;
    StringValue* value = Value::CreateStringValue(...);
    if (email_address.primary())
      CHECK(email_list->Insert(0, value));
    else
      email_list->Append(value);
  }

?

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:113: if
(!command_line->HasSwitch(switches::kEnableContacts))
is this something that you actually need to check here?  we won't even be
loading any contacts, let alone passing them to this code, when the flag is off.

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.h (right):

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.h:15: class
RtcPrivateApi {
nit: add a comment describing what this is for, e.g.

// Used to establish a real-time communication channel with a contact.

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.h:30: }  // namespace
chromeos
nit: fix comment

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
File chrome/browser/extensions/api/rtc_private/rtc_private_apitest.cc (right):

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_apitest.cc:39: } OVERRIDE;
i think that this should come before the curly brackets, not after

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_apitest.cc:41: static
contacts::Contact* GetTestContact() {
nit: rename to CreateTestContact() to make it clear that this is allocating a
new contact instead of just returning an existing one that's owned by something
else

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_apitest.cc:63: 
nit: remove blank line

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/chrome_brows...
File chrome/chrome_browser_extensions.gypi (right):

https://chromiumcodereview.appspot.com/10919086/diff/4004/chrome/chrome_brows...
chrome/chrome_browser_extensions.gypi:229:
'browser/extensions/api/rtc_private/rtc_private_api.cc',
nit: alphabetize

[zel, 2012-09-05 19:56:02]

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.cc (right):

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:11: #include
"base/stringprintf.h"
On 2012/09/05 19:28:32, Daniel Erat wrote:
> nit: alphabetize

Done.

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:73: break;
On 2012/09/05 19:28:32, Daniel Erat wrote:
> nit: add a default case with NOTREACHED() << "Unknown action " << action;

Done.

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:80:
scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
On 2012/09/05 19:28:32, Daniel Erat wrote:
> nit: any reason that you actually need a scoped_ptr here instead of just
> allocating this on the stack?

Done.

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:88: for (i = 0; i <
contact->phone_numbers_size(); i++) {
On 2012/09/05 19:28:32, Daniel Erat wrote:
> nit: move 'i' declaration into the loop condition

Done.

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:95: for (i = 0; i <
contact->email_addresses_size(); i++) {
On 2012/09/05 19:28:32, Daniel Erat wrote:
> we talked earlier about sorting phone numbers and email addresses so the entry
> with its 'primary' field set to true will come first.  how about doing
something
> like this instead:
> 
>   for (...) {
>     const Contact_EmailAddress& email_address = ...;
>     StringValue* value = Value::CreateStringValue(...);
>     if (email_address.primary())
>       CHECK(email_list->Insert(0, value));
>     else
>       email_list->Append(value);
>   }
> 
> ?

Done.

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:113: if
(!command_line->HasSwitch(switches::kEnableContacts))
On 2012/09/05 19:28:32, Daniel Erat wrote:
> is this something that you actually need to check here?  we won't even be
> loading any contacts, let alone passing them to this code, when the flag is
off.

Fair enough. Removed.

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.h (right):

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
chrome/browser/extensions/api/rtc_private/rtc_private_api.h:15: class
RtcPrivateApi {
On 2012/09/05 19:28:32, Daniel Erat wrote:
> nit: add a comment describing what this is for, e.g.
> 
> // Used to establish a real-time communication channel with a contact.

Done.

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
chrome/browser/extensions/api/rtc_private/rtc_private_api.h:30: }  // namespace
chromeos
On 2012/09/05 19:28:32, Daniel Erat wrote:
> nit: fix comment

Done.

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
File chrome/browser/extensions/api/rtc_private/rtc_private_apitest.cc (right):

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
chrome/browser/extensions/api/rtc_private/rtc_private_apitest.cc:39: } OVERRIDE;
On 2012/09/05 19:28:32, Daniel Erat wrote:
> i think that this should come before the curly brackets, not after

Done.

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
chrome/browser/extensions/api/rtc_private/rtc_private_apitest.cc:41: static
contacts::Contact* GetTestContact() {
On 2012/09/05 19:28:32, Daniel Erat wrote:
> nit: rename to CreateTestContact() to make it clear that this is allocating a
> new contact instead of just returning an existing one that's owned by
something
> else

Done.

http://codereview.chromium.org/10919086/diff/4004/chrome/browser/extensions/a...
chrome/browser/extensions/api/rtc_private/rtc_private_apitest.cc:63: 
On 2012/09/05 19:28:32, Daniel Erat wrote:
> nit: remove blank line

Done.

http://codereview.chromium.org/10919086/diff/4004/chrome/chrome_browser_exten...
File chrome/chrome_browser_extensions.gypi (right):

http://codereview.chromium.org/10919086/diff/4004/chrome/chrome_browser_exten...
chrome/chrome_browser_extensions.gypi:229:
'browser/extensions/api/rtc_private/rtc_private_api.cc',
On 2012/09/05 19:28:32, Daniel Erat wrote:
> nit: alphabetize

yeah, my sorting order algo obviously needs improvements ;)

Done.

[Daniel Erat, 2012-09-05 20:06:03]

LGTM if others are happy with the API side of this.

[Mihai Parparita -not on Chrome, 2012-09-05 20:11:09]

http://codereview.chromium.org/10919086/diff/9002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.cc (right):

http://codereview.chromium.org/10919086/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:51: const char
kIntentField[] = "intent";
There should be a LaunchIntent struct that gets auto-generated that you can
populate, instead of having to repeat the field names here (see
http://crrev.com/147954 for a recent change that switched to using the
auto-generated code).

http://codereview.chromium.org/10919086/diff/9002/chrome/common/extensions/ap...
File chrome/common/extensions/api/rtc_private.idl (right):

http://codereview.chromium.org/10919086/diff/9002/chrome/common/extensions/ap...
chrome/common/extensions/api/rtc_private.idl:8: DOMString action;
This should use an enum.

[Dmitry Titov, 2012-09-05 21:07:31]

https://chromiumcodereview.appspot.com/10919086/diff/10018/chrome/common/exte...
File chrome/common/extensions/api/_permission_features.json (right):

https://chromiumcodereview.appspot.com/10919086/diff/10018/chrome/common/exte...
chrome/common/extensions/api/_permission_features.json:239:
"ljclpkphhpbpinifbeabbhlfddcpfdde"  // Google Talk
There are 4 IDs for GTalk (debug/alpha/beta/prod)
I'll send you email with a reference.

[zel, 2012-09-05 21:30:44]

https://chromiumcodereview.appspot.com/10919086/diff/9002/chrome/browser/exte...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.cc (right):

https://chromiumcodereview.appspot.com/10919086/diff/9002/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:51: const char
kIntentField[] = "intent";
On 2012/09/05 20:11:09, Mihai Parparita wrote:
> There should be a LaunchIntent struct that gets auto-generated that you can
> populate, instead of having to repeat the field names here (see
> http://crrev.com/147954 for a recent change that switched to using the
> auto-generated code).

Done.

https://chromiumcodereview.appspot.com/10919086/diff/9002/chrome/common/exten...
File chrome/common/extensions/api/rtc_private.idl (right):

https://chromiumcodereview.appspot.com/10919086/diff/9002/chrome/common/exten...
chrome/common/extensions/api/rtc_private.idl:8: DOMString action;
On 2012/09/05 20:11:09, Mihai Parparita wrote:
> This should use an enum.

Done.

https://chromiumcodereview.appspot.com/10919086/diff/10018/chrome/common/exte...
File chrome/common/extensions/api/_permission_features.json (right):

https://chromiumcodereview.appspot.com/10919086/diff/10018/chrome/common/exte...
chrome/common/extensions/api/_permission_features.json:239:
"ljclpkphhpbpinifbeabbhlfddcpfdde"  // Google Talk
On 2012/09/05 21:07:31, Dmitry Titov wrote:
> There are 4 IDs for GTalk (debug/alpha/beta/prod)
> I'll send you email with a reference.

Done.

[Dmitry Titov, 2012-09-05 21:47:47]

LGTM for quasar, with nits:

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.cc (right):

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:39: const char
kDefaultRtcExtensionId[] = "ljclpkphhpbpinifbeabbhlfddcpfdde";
This seems not used.

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/common/chrom...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/common/chrom...
chrome/common/chrome_switches.cc:494: const char kTestRtcExtensionId[]          
 = "test-rtc-extension";
Do you still need this switch?

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/test/data/ex...
File chrome/test/data/extensions/api_test/rtc_private/events/background.js
(right):

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/test/data/ex...
chrome/test/data/extensions/api_test/rtc_private/events/background.js:15: if
(action == 'chat' || action == 'video' || action == 'voice') {
The test sends emails and phone numbers, but the background page does not test
that they actually come in 'intent.data', just logs them to the console... Is it
possible to add the check?

[Mihai Parparita -not on Chrome, 2012-09-05 22:01:10]

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.cc (right):

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:76: std::string
GetContactIntentData(Contact* contact) {
Does this have to be a string? Normally intent data can be a JavaScript object
too (anything that can be passed via
http://www.w3.org/TR/html5/common-dom-interfaces.html#safe-passing-of-structu...).
So it could be returned as a DictionaryValue (or if you want to make it even
more structured, you could have the contact be a type defined by the IDL).

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.h (right):

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.h:30: static void
RaiseLaunchEvent(Profile* profile,
Sorry for not catching this earlier, but the pattern for event dispatching in
extension code is to 1) have the code be in an event router class 2) have the
method be called Dispatch<Event>. So this should be
RtcPrivateEventRouter::DispatchLaunchEvent

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
File chrome/browser/extensions/api/rtc_private/rtc_private_apitest.cc (right):

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_apitest.cc:89: // Collect
results to make sure all web intents have been received on
Won't this be racy? You're not actually waiting for events to be delivered
before checking for responses.

A better approach is to use a ExtensionTestMessageListener that you
WaitUntilSatisfied() for each event (in the background page you would only call
chrome.test.sendMessage if the received event was that you expected). That way
you won't need something like check_events.html at all.

[zel, 2012-09-05 22:06:21]

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.cc (right):

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:39: const char
kDefaultRtcExtensionId[] = "ljclpkphhpbpinifbeabbhlfddcpfdde";
On 2012/09/05 21:47:47, Dmitry Titov wrote:
> This seems not used.

Done.

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/common/chrom...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/common/chrom...
chrome/common/chrome_switches.cc:494: const char kTestRtcExtensionId[]          
 = "test-rtc-extension";
On 2012/09/05 21:47:47, Dmitry Titov wrote:
> Do you still need this switch?

Done.

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/test/data/ex...
File chrome/test/data/extensions/api_test/rtc_private/events/background.js
(right):

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/test/data/ex...
chrome/test/data/extensions/api_test/rtc_private/events/background.js:15: if
(action == 'chat' || action == 'video' || action == 'voice') {
On 2012/09/05 21:47:47, Dmitry Titov wrote:
> The test sends emails and phone numbers, but the background page does not test
> that they actually come in 'intent.data', just logs them to the console... Is
it
> possible to add the check?

Done.

[zel, 2012-09-05 22:28:26]

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.cc (right):

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.cc:76: std::string
GetContactIntentData(Contact* contact) {
On 2012/09/05 22:01:10, Mihai Parparita wrote:
> Does this have to be a string? Normally intent data can be a JavaScript object
> too (anything that can be passed via
>
http://www.w3.org/TR/html5/common-dom-interfaces.html#safe-passing-of-structu...).
> So it could be returned as a DictionaryValue (or if you want to make it even
> more structured, you could have the contact be a type defined by the IDL).

Done.

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
File chrome/browser/extensions/api/rtc_private/rtc_private_api.h (right):

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_api.h:30: static void
RaiseLaunchEvent(Profile* profile,
On 2012/09/05 22:01:10, Mihai Parparita wrote:
> Sorry for not catching this earlier, but the pattern for event dispatching in
> extension code is to 1) have the code be in an event router class 2) have the
> method be called Dispatch<Event>. So this should be
> RtcPrivateEventRouter::DispatchLaunchEvent

Done.

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
File chrome/browser/extensions/api/rtc_private/rtc_private_apitest.cc (right):

https://chromiumcodereview.appspot.com/10919086/diff/2020/chrome/browser/exte...
chrome/browser/extensions/api/rtc_private/rtc_private_apitest.cc:89: // Collect
results to make sure all web intents have been received on
On 2012/09/05 22:01:10, Mihai Parparita wrote:
> Won't this be racy? You're not actually waiting for events to be delivered
> before checking for responses.
> 
> A better approach is to use a ExtensionTestMessageListener that you
> WaitUntilSatisfied() for each event (in the background page you would only
call
> chrome.test.sendMessage if the received event was that you expected). That way
> you won't need something like check_events.html at all.

Done.

[Mihai Parparita -not on Chrome, 2012-09-05 22:35:29]

LGTM

https://chromiumcodereview.appspot.com/10919086/diff/5070/chrome/test/data/ex...
File chrome/test/data/extensions/api_test/rtc_private/events/check_events.js
(right):

https://chromiumcodereview.appspot.com/10919086/diff/5070/chrome/test/data/ex...
chrome/test/data/extensions/api_test/rtc_private/events/check_events.js:6: if
(response.count != 4) {
This will never not be 4, since the message to unblock the waiting is not sent
until it's 4. I don't think you need this code anymore.

[zel, 2012-09-05 23:33:48]

https://chromiumcodereview.appspot.com/10919086/diff/5070/chrome/test/data/ex...
File chrome/test/data/extensions/api_test/rtc_private/events/check_events.js
(right):

https://chromiumcodereview.appspot.com/10919086/diff/5070/chrome/test/data/ex...
chrome/test/data/extensions/api_test/rtc_private/events/check_events.js:6: if
(response.count != 4) {
On 2012/09/05 22:35:29, Mihai Parparita wrote:
> This will never not be 4, since the message to unblock the waiting is not sent
> until it's 4. I don't think you need this code anymore.

Done. Removed check_events.* and related code.

[commit-bot: I haz the power, 2012-09-05 23:34:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/10919086/10020

[commit-bot: I haz the power, 2012-09-05 23:34:16]

Presubmit check for 10919086-10020 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[sky, 2012-09-05 23:37:10]

LGTM

[commit-bot: I haz the power, 2012-09-05 23:38:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/10919086/10020

[commit-bot: I haz the power, 2012-09-06 03:42:52]

Change committed as 155114

[commit-bot: I haz the power, 2012-09-06 18:09:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/10919086/3027

[commit-bot: I haz the power, 2012-09-06 20:19:20]

Try job failure for 10919086-3027 (retry) on mac_rel for step
"sync_integration_tests".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...