Passing NULL PeerConnectionIdentityService to Libjingle when Chrome is using openssl.

Passing NULL PeerConnectionIdentityService to Libjingle when Chrome is using openssl.
Because the crypto APIs needed for generating identities are not implemented for openssl yet, we should let Libjingle generate its own by passing NULL.

BUG=

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

[jiayl, 2013-08-09 17:01:02]

PTAL. Thanks!

[Ami GONE FROM CHROMIUM, 2013-08-09 17:14:34]

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

https://codereview.chromium.org/22640017/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:557: #if
!defined(USE_OPENSSL)
I don't really understand this.  Would it be useful to file a crbug (and point
to it from here) that enumerates the missing bits from openssl?

(should PCIS expose the fact that it is useless because of lack of underlying
impl details?  It seems strange to have to guard for this condition from here)

[jiayl, 2013-08-09 17:41:16]

PTAL

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

https://codereview.chromium.org/22640017/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:557: #if
!defined(USE_OPENSSL)
On 2013/08/09 17:14:35, Ami Fischman wrote:
> I don't really understand this.  Would it be useful to file a crbug (and point
> to it from here) that enumerates the missing bits from openssl?
> 
> (should PCIS expose the fact that it is useless because of lack of underlying
> impl details?  It seems strange to have to guard for this condition from here)

Added the link to the existing bug.
I feel it's a cleaner interaction with Libjingle if NULL means "generate by
yourself" and non-NULL means "I can do it for you", instead of making Libjingle
checking error codes, which come from a header in Chrome.

[Ami GONE FROM CHROMIUM, 2013-08-09 18:11:22]

On 2013/08/09 17:41:16, jiayl wrote:
> PTAL
> 
>
https://codereview.chromium.org/22640017/diff/1/content/renderer/media/media_...
> File content/renderer/media/media_stream_dependency_factory.cc (right):
> 
>
https://codereview.chromium.org/22640017/diff/1/content/renderer/media/media_...
> content/renderer/media/media_stream_dependency_factory.cc:557: #if
> !defined(USE_OPENSSL)
> On 2013/08/09 17:14:35, Ami Fischman wrote:
> > I don't really understand this.  Would it be useful to file a crbug (and
point
> > to it from here) that enumerates the missing bits from openssl?
> > 
> > (should PCIS expose the fact that it is useless because of lack of
underlying
> > impl details?  It seems strange to have to guard for this condition from
here)
> 
> Added the link to the existing bug.

That bug doesn't seem likely to be fixed anytime soon.  Are you planning to take
it on?

> I feel it's a cleaner interaction with Libjingle if NULL means "generate by
> yourself" and non-NULL means "I can do it for you", instead of making
Libjingle
> checking error codes, which come from a header in Chrome.

I was not suggesting this.  I was thinking of something more like:
replace the |new PCIS()| call with a static factory PCIS::Create() method that
returned a constructed PCIS if such a PCIS could be useful (i.e. !openssl) or
NULL if it couldn't be useful.

My motivation is that the knowledge that PCIS is neutered in openssl builds
shouldn't belong anywhere outside PCIS itself.

[jiayl, 2013-08-09 18:42:02]

PTAL. Thanks!

On 2013/08/09 18:11:22, Ami Fischman wrote:
> On 2013/08/09 17:41:16, jiayl wrote:
> > PTAL
> > 
> >
>
https://codereview.chromium.org/22640017/diff/1/content/renderer/media/media_...
> > File content/renderer/media/media_stream_dependency_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/22640017/diff/1/content/renderer/media/media_...
> > content/renderer/media/media_stream_dependency_factory.cc:557: #if
> > !defined(USE_OPENSSL)
> > On 2013/08/09 17:14:35, Ami Fischman wrote:
> > > I don't really understand this.  Would it be useful to file a crbug (and
> point
> > > to it from here) that enumerates the missing bits from openssl?
> > > 
> > > (should PCIS expose the fact that it is useless because of lack of
> underlying
> > > impl details?  It seems strange to have to guard for this condition from
> here)
> > 
> > Added the link to the existing bug.
> 
> That bug doesn't seem likely to be fixed anytime soon.  Are you planning to
take
> it on?
I don't have a plan now. If it's our priority, I can reach out to them to find
what are our options.

> > I feel it's a cleaner interaction with Libjingle if NULL means "generate by
> > yourself" and non-NULL means "I can do it for you", instead of making
> Libjingle
> > checking error codes, which come from a header in Chrome.
> 
> I was not suggesting this.  I was thinking of something more like:
> replace the |new PCIS()| call with a static factory PCIS::Create() method that
> returned a constructed PCIS if such a PCIS could be useful (i.e. !openssl) or
> NULL if it couldn't be useful.
> 
> My motivation is that the knowledge that PCIS is neutered in openssl builds
> shouldn't belong anywhere outside PCIS itself.
I see. Fixed now.

[Ami GONE FROM CHROMIUM, 2013-08-09 18:58:58]

> I don't have a plan now. If it's our priority, I can reach out to them to find
what are our options.

I don't know how high a priority this is; I expected you to know :)
Chrome/Android uses openssl, so perf concerns seem extra-important there.

https://codereview.chromium.org/22640017/diff/7001/content/renderer/media/pee...
File content/renderer/media/peer_connection_identity_service.cc (right):

https://codereview.chromium.org/22640017/diff/7001/content/renderer/media/pee...
content/renderer/media/peer_connection_identity_service.cc:15: // OPENSSL yet
(crbug/91512). So passing NULL to let Libjingle generate its own.
Comment needs to be updated.

[Ami GONE FROM CHROMIUM, 2013-08-09 18:59:04]

LGTM % comment

[jiayl, 2013-08-09 20:07:00]

On 2013/08/09 18:58:58, Ami Fischman wrote:
> > I don't have a plan now. If it's our priority, I can reach out to them to
find
> what are our options.
> 
> I don't know how high a priority this is; I expected you to know :)
> Chrome/Android uses openssl, so perf concerns seem extra-important there.
> 
>
https://codereview.chromium.org/22640017/diff/7001/content/renderer/media/pee...
> File content/renderer/media/peer_connection_identity_service.cc (right):
> 
>
https://codereview.chromium.org/22640017/diff/7001/content/renderer/media/pee...
> content/renderer/media/peer_connection_identity_service.cc:15: // OPENSSL yet
> (crbug/91512). So passing NULL to let Libjingle generate its own.
> Comment needs to be updated.

Done

[commit-bot: I haz the power, 2013-08-09 20:12:09]

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

[juberti2, 2013-08-09 20:23:39]

Agree the perf considerations here will be even more significant on mobile.

https://codereview.chromium.org/22640017/diff/14001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/22640017/diff/14001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:554:
PeerConnectionIdentityService::Create(
Suggest adding a comment to indicate that a NULL return is not an error.

[juberti2, 2013-08-09 20:23:51]

On 2013/08/09 20:23:39, juberti2 wrote:
> Agree the perf considerations here will be even more significant on mobile.
> 
>
https://codereview.chromium.org/22640017/diff/14001/content/renderer/media/me...
> File content/renderer/media/media_stream_dependency_factory.cc (right):
> 
>
https://codereview.chromium.org/22640017/diff/14001/content/renderer/media/me...
> content/renderer/media/media_stream_dependency_factory.cc:554:
> PeerConnectionIdentityService::Create(
> Suggest adding a comment to indicate that a NULL return is not an error.

lgtm

[commit-bot: I haz the power, 2013-08-10 02:50:50]

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

[commit-bot: I haz the power, 2013-08-10 04:10:01]

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

[commit-bot: I haz the power, 2013-08-10 13:38:50]

Change committed as 216837