Polishing code to handle certificate generation failure in .mm files.

Polishing code to handle certificate generation failure in .mm files.
This is a follow-up to https://codereview.webrtc.org/1965313002/ which
was TBR-landed.

Minor code clean-up/corrections:

Property nativeConfiguration -> - method createNativeConfiguration.
RTCLogWarning -> RTCLogError.
setConfiguration returning NO instead of false.
initWithFactory returning nil instead of nullptr.
Braces around ifs.

Committed: https://crrev.com/a73ca5668ec2bc8c54e65d8fb9bf85982089d045
Cr-Commit-Position: refs/heads/master@{#12770}

[hbos, 2016-05-16 12:47:06]

Description was changed from

==========
Polishing code to handle certificate generation failure in .mm files.
This is a follow-up to https://codereview.webrtc.org/1965313002/ which
was TBR-landed.

Property nativeConfiguration -> - method createNativeConfiguration.
setConfiguration returning NO instead of false.
initWithFactory returning nil instead of nullptr.
==========

to

==========
Polishing code to handle certificate generation failure in .mm files.
This is a follow-up to https://codereview.webrtc.org/1965313002/ which
was TBR-landed.

Minor code clean-up/corrections:

Property nativeConfiguration -> - method createNativeConfiguration.
RTCLogWarning -> RTCLogError.
setConfiguration returning NO instead of false.
initWithFactory returning nil instead of nullptr.
==========

[hbos, 2016-05-16 12:50:54]

Comments and replies to them on the CL that has already landed:
https://codereview.webrtc.org/1965313002/

---

https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h (right):

https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h:24:
webrtc::PeerConnectionInterface::RTCConfiguration* nativeConfiguration;
On 2016/05/13 17:48:05, tkchin_webrtc wrote:
> RTCConfiguration *nativeConfiguration;
> 
> This should be a method instead of a property because it is actually creating
an
> object that needs to be released or at the very least commented to indicate
that
> manual memory management is required. The style is to prefix with "create".
> 
> - (webrtc::PeerConnectionInterface::RTCConfiguration
> *)createNativeConfiguration;

Done.

https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm (right):

https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:78: -
(webrtc::PeerConnectionInterface::RTCConfiguration*)nativeConfiguration {
On 2016/05/13 17:48:05, tkchin_webrtc wrote:
> RTCConfiguration *)

Done.

https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:79:
std::unique_ptr<webrtc::PeerConnectionInterface::RTCConfiguration>
On 2016/05/13 18:07:38, tkchin_webrtc wrote:
> On 2016/05/13 17:48:05, tkchin_webrtc wrote:
> > Are we returning a pointer to avoid memory copy? Or to surface an error with
> > certificate generation? Why don't we want a default fallback?

To say that we failed to generate the configuration asked for (surface
certificate failure).
As for attempting to generate a different certificate as a fallback, if you
choose ECDSA over RSA for security reasons it may(?) be bad if you implicitly
end up with an RSA certificate.

> > This will cause RTCPeerConnectionFactory to potentially return nil on init,
> > which isn't great because it's like saying your constructor failed.
> > We generally reserve returning nil on init for fairly exceptional
> circumstances
> > - when is certificate generation expected to fail?

Under very rare circumstances. I don't know how to make it fail or if this ever
happens in practice. Our unittests EXPECT generate certificate to return a
certificate and I haven't ever seen the unittest failing to produce one. But the
lower-level crypto libraries could return null and that is propagated by the
interface instead of crashing.

> > It may be better for RTCConfiguration to store a copy of the C++ object on
> init,
> > so RTCConfiguration is the object that returns nil on init instead, which
> makes
> > more sense, and will be caught by assertions in RTCPeerConnectionFactory.
> 
> and by factory I meant peerconnection's private
initWithFactory:configuration...
> method :)
> which I guess isn't so bad because it's private and within our control.

Do you mean to move the GenerateCertificate code from createNativeConfiguration
to RTCConfiguration's init? That would make _keyType pointless, and it should
then be removed or be made const? If we don't move the code we have the option
to modify the default value of _keyType to support other certificates.

In both cases we end up with an init that could conceivably return nil.
Alternatively we could add an assertion that GenerateCertificate does not fail
and change the code back so that we do not propagate the error.

What do you think I should do? Currently I did not change this and still have
createNativeConfiguration.

https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCConfiguration.mm:108:
RTCLogWarning(@"Failed to generate certificate.");
On 2016/05/13 18:07:38, tkchin_webrtc wrote:
> On 2016/05/13 17:48:05, tkchin_webrtc wrote:
> > this is an error because you will fail to create things like factory
> > RTCLogError
> 
> I meant peerconnection via [factory peerconnectionWithConfiguration..]

RTCLogError - Done.

https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right):

https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:203: return nullptr;
On 2016/05/13 17:48:06, tkchin_webrtc wrote:
> ObjC style requires braces
> 
> nil not nullptr. 
> if (!nativeConfiguration) {
>   return nil;
> }

Done.

https://codereview.webrtc.org/1965313002/diff/60001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:259: return false;
On 2016/05/13 17:48:06, tkchin_webrtc wrote:
> BOOL is not a boolean
> 
> if (!nativeConfiguration) {
>   return NO;
> }

Done.

[hbos, 2016-05-16 12:53:49]

hbos@webrtc.org changed reviewers:
+ tkchin@webrtc.org

[hbos, 2016-05-16 12:53:49]

Please take a look tkchin.

[tkchin_webrtc, 2016-05-16 21:01:33]

Thanks for addressing comments.

lgtm % nits

Re: returning nil on init, let's just leave it for now since it's exceptional
and it's in a private ctor that only gets called in the factory anyway.

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h (right):

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h:24:
createNativeConfiguration;
nit: instance methods below static ones (so place below
stringForTcpCandidatePolicy)

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right):

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:201:
configuration.createNativeConfiguration);
not a property, so use [configuration createNativeConfiguration]

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:202: if (!config)
braces

if (!config) {
  return nil;
}

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:257:
configuration.createNativeConfiguration);
ditto [] and {}

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Uni...
File webrtc/sdk/objc/Framework/UnitTests/RTCConfigurationTest.mm (right):

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Uni...
webrtc/sdk/objc/Framework/UnitTests/RTCConfigurationTest.mm:48:
nativeConfig(config.createNativeConfiguration);
[]

[hbos, 2016-05-17 09:04:10]

Description was changed from

==========
Polishing code to handle certificate generation failure in .mm files.
This is a follow-up to https://codereview.webrtc.org/1965313002/ which
was TBR-landed.

Minor code clean-up/corrections:

Property nativeConfiguration -> - method createNativeConfiguration.
RTCLogWarning -> RTCLogError.
setConfiguration returning NO instead of false.
initWithFactory returning nil instead of nullptr.
==========

to

==========
Polishing code to handle certificate generation failure in .mm files.
This is a follow-up to https://codereview.webrtc.org/1965313002/ which
was TBR-landed.

Minor code clean-up/corrections:

Property nativeConfiguration -> - method createNativeConfiguration.
RTCLogWarning -> RTCLogError.
setConfiguration returning NO instead of false.
initWithFactory returning nil instead of nullptr.
Braces around ifs.
==========

[hbos, 2016-05-17 09:05:03]

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h (right):

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCConfiguration+Private.h:24:
createNativeConfiguration;
On 2016/05/16 21:01:32, tkchin_webrtc wrote:
> nit: instance methods below static ones (so place below
> stringForTcpCandidatePolicy)

Done.

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm (right):

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:201:
configuration.createNativeConfiguration);
On 2016/05/16 21:01:32, tkchin_webrtc wrote:
> not a property, so use [configuration createNativeConfiguration]

Done. *Pretending to know objective-c++ when I don't*

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:202: if (!config)
On 2016/05/16 21:01:32, tkchin_webrtc wrote:
> braces
> 
> if (!config) {
>   return nil;
> }

Done. (I think you asked me in the other CL and I missed it.)

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm:257:
configuration.createNativeConfiguration);
On 2016/05/16 21:01:32, tkchin_webrtc wrote:
> ditto [] and {}

Done.

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Uni...
File webrtc/sdk/objc/Framework/UnitTests/RTCConfigurationTest.mm (right):

https://codereview.webrtc.org/1978233002/diff/1/webrtc/sdk/objc/Framework/Uni...
webrtc/sdk/objc/Framework/UnitTests/RTCConfigurationTest.mm:48:
nativeConfig(config.createNativeConfiguration);
On 2016/05/16 21:01:32, tkchin_webrtc wrote:
> []

Done.

[hbos, 2016-05-17 09:05:25]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2016-05-17 09:05:25]

The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1978233002/#ps20001 (title:
"Addressed nits")

[commit-bot: I haz the power, 2016-05-17 09:05:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1978233002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1978233002/20001

[commit-bot: I haz the power, 2016-05-17 10:29:02]

Description was changed from

==========
Polishing code to handle certificate generation failure in .mm files.
This is a follow-up to https://codereview.webrtc.org/1965313002/ which
was TBR-landed.

Minor code clean-up/corrections:

Property nativeConfiguration -> - method createNativeConfiguration.
RTCLogWarning -> RTCLogError.
setConfiguration returning NO instead of false.
initWithFactory returning nil instead of nullptr.
Braces around ifs.
==========

to

==========
Polishing code to handle certificate generation failure in .mm files.
This is a follow-up to https://codereview.webrtc.org/1965313002/ which
was TBR-landed.

Minor code clean-up/corrections:

Property nativeConfiguration -> - method createNativeConfiguration.
RTCLogWarning -> RTCLogError.
setConfiguration returning NO instead of false.
initWithFactory returning nil instead of nullptr.
Braces around ifs.
==========

[commit-bot: I haz the power, 2016-05-17 10:29:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-17 10:29:17]

Description was changed from

==========
Polishing code to handle certificate generation failure in .mm files.
This is a follow-up to https://codereview.webrtc.org/1965313002/ which
was TBR-landed.

Minor code clean-up/corrections:

Property nativeConfiguration -> - method createNativeConfiguration.
RTCLogWarning -> RTCLogError.
setConfiguration returning NO instead of false.
initWithFactory returning nil instead of nullptr.
Braces around ifs.
==========

to

==========
Polishing code to handle certificate generation failure in .mm files.
This is a follow-up to https://codereview.webrtc.org/1965313002/ which
was TBR-landed.

Minor code clean-up/corrections:

Property nativeConfiguration -> - method createNativeConfiguration.
RTCLogWarning -> RTCLogError.
setConfiguration returning NO instead of false.
initWithFactory returning nil instead of nullptr.
Braces around ifs.

Committed: https://crrev.com/a73ca5668ec2bc8c54e65d8fb9bf85982089d045
Cr-Commit-Position: refs/heads/master@{#12770}
==========

[commit-bot: I haz the power, 2016-05-17 10:29:18]

Patchset 2 (id:??) landed as
https://crrev.com/a73ca5668ec2bc8c54e65d8fb9bf85982089d045
Cr-Commit-Position: refs/heads/master@{#12770}