Add video codec setting to AppRTCMobile on iOS.

Add video codec setting to AppRTCMobile on iOS.

List items in settings menu no longer stay selected. Checkmarks are now
added to the selected options before the view appears.

BUG=webrtc:7316
TBR=denicija

Review-Url: https://codereview.webrtc.org/2735303004
Cr-Commit-Position: refs/heads/master@{#17296}
Committed: https://chromium.googlesource.com/external/webrtc/+/68b5df97c77a1ae5e16ec27f83cbce098411da0e

[sakal, 2017-03-09 12:30:04]

Patchset #1 (id:1) has been deleted

[sakal, 2017-03-09 12:30:30]

sakal@webrtc.org changed reviewers:
+ kthelgason@webrtc.org, magjed@webrtc.org

[sakal, 2017-03-09 12:35:39]

Patchset #1 (id:20001) has been deleted

[sakal, 2017-03-09 12:38:35]

Description was changed from

==========
Add video codec setting to AppRTCMobile on iOS.
==========

to

==========
Add video codec setting to AppRTCMobile on iOS.

List items in settings menu no longer stay selected. Checkmarks are now
added to the selected options before the view appears.
==========

[sakal, 2017-03-09 12:38:43]

PTAL

[sakal, 2017-03-09 12:43:06]

Description was changed from

==========
Add video codec setting to AppRTCMobile on iOS.

List items in settings menu no longer stay selected. Checkmarks are now
added to the selected options before the view appears.
==========

to

==========
Add video codec setting to AppRTCMobile on iOS.

List items in settings menu no longer stay selected. Checkmarks are now
added to the selected options before the view appears.

BUG=webrtc:7316
==========

[kthelgason, 2017-03-09 13:39:43]

Some comments:

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/ARDAppClient.m (right):

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ARDAppClient.m:102: NSString *_videoCodec;
Maybe make this a property and @synthesize it. Then you get the setter for free.

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsModel.m (right):

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsModel.m:57: - (nullable
NSString *)currentVideoCodecSettingFromStore {
AFAIKT we don't ever store null so this needs not be nullable.

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsModel.m:66: -
(BOOL)storeVideoCodecSetting:(nullable NSString *)videoCodec {
should this be nullable?

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.h (right):

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.h:36: - (nullable
NSString *)videoCodec;
not nullable?

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.h:42: -
(void)setVideoCodec:(nullable NSString *)value;
ditto

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m (right):

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:50: [self
decorateCurrentlyStoredOrDefaultVideoCodec];
Is this safe to do here? Why not in viewDidAppear?

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:115: }
I'd prefer a switch-case on section here.

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:179: if ([self
indexPathIsMediaConstraints:indexPath]) {
I'd also prefer a switch-case here, but up to you.

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:243: }
It seems to me that there is a lot of shared code with the Media Costraints.
Perhaps we can refactor this to reduce the duplication?

[sakal, 2017-03-10 09:46:27]

Patchset #2 (id:60001) has been deleted

[sakal, 2017-03-10 10:25:28]

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/ARDAppClient.m (right):

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ARDAppClient.m:102: NSString *_videoCodec;
On 2017/03/09 13:39:43, kthelgason wrote:
> Maybe make this a property and @synthesize it. Then you get the setter for
free.

Done.

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsModel.m (right):

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsModel.m:57: - (nullable
NSString *)currentVideoCodecSettingFromStore {
On 2017/03/09 13:39:43, kthelgason wrote:
> AFAIKT we don't ever store null so this needs not be nullable.

Done.

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsModel.m:66: -
(BOOL)storeVideoCodecSetting:(nullable NSString *)videoCodec {
On 2017/03/09 13:39:43, kthelgason wrote:
> should this be nullable?

Done.

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.h (right):

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.h:36: - (nullable
NSString *)videoCodec;
On 2017/03/09 13:39:43, kthelgason wrote:
> not nullable?

Actually I can make this property as well.

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.h:42: -
(void)setVideoCodec:(nullable NSString *)value;
On 2017/03/09 13:39:43, kthelgason wrote:
> ditto

Done.

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m (right):

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:50: [self
decorateCurrentlyStoredOrDefaultVideoCodec];
On 2017/03/09 13:39:43, kthelgason wrote:
> Is this safe to do here? Why not in viewDidAppear?

I didn't notice any problems in my testing. If we do this in viewDidAppear, the
checkmarks appear after the view has animated to the place, which is ugly.

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:115: }
On 2017/03/09 13:39:43, kthelgason wrote:
> I'd prefer a switch-case on section here.

Done.

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:179: if ([self
indexPathIsMediaConstraints:indexPath]) {
On 2017/03/09 13:39:43, kthelgason wrote:
> I'd also prefer a switch-case here, but up to you.

Done.

https://codereview.webrtc.org/2735303004/diff/40001/webrtc/examples/objc/AppR...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:243: }
On 2017/03/09 13:39:43, kthelgason wrote:
> It seems to me that there is a lot of shared code with the Media Costraints.
> Perhaps we can refactor this to reduce the duplication?

Done.

[kthelgason, 2017-03-10 10:41:44]

The refactoring looks really good! One suggestion and some nits.

https://codereview.webrtc.org/2735303004/diff/100001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.h (right):

https://codereview.webrtc.org/2735303004/diff/100001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.h:22:
@property(getter=videoCodec) NSString* videoCodec;
This is the default name of the getter, specifying it is not necessary.

https://codereview.webrtc.org/2735303004/diff/100001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m (right):

https://codereview.webrtc.org/2735303004/diff/100001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:50:
withArray:[self mediaConstraintsArray]
is mediaConstraintsArray a method or a property? We prefer dot-syntax for
properties.

https://codereview.webrtc.org/2735303004/diff/100001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:53:
withArray:[self videoCodecArray]
ditto

https://codereview.webrtc.org/2735303004/diff/100001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m (right):

https://codereview.webrtc.org/2735303004/diff/100001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m:54: [_client
setVideoCodec:[settingsModel currentVideoCodecSettingFromStore]];
I realize that this is just copying the style below but it should rather be
`client.videoCodec =` for properties.

Also, consider making the property read-only and passing it in the initializer.
Seems safer to me since we make the assumption in ARDAppClient that it's always
set.

[sakal, 2017-03-10 11:23:14]

https://codereview.webrtc.org/2735303004/diff/100001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.h (right):

https://codereview.webrtc.org/2735303004/diff/100001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.h:22:
@property(getter=videoCodec) NSString* videoCodec;
On 2017/03/10 10:41:44, kthelgason wrote:
> This is the default name of the getter, specifying it is not necessary.

Done.

https://codereview.webrtc.org/2735303004/diff/100001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m (right):

https://codereview.webrtc.org/2735303004/diff/100001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:50:
withArray:[self mediaConstraintsArray]
On 2017/03/10 10:41:44, kthelgason wrote:
> is mediaConstraintsArray a method or a property? We prefer dot-syntax for
> properties.

They are not declared as properties.

https://codereview.webrtc.org/2735303004/diff/100001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m (right):

https://codereview.webrtc.org/2735303004/diff/100001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDVideoCallViewController.m:54: [_client
setVideoCodec:[settingsModel currentVideoCodecSettingFromStore]];
On 2017/03/10 10:41:44, kthelgason wrote:
> I realize that this is just copying the style below but it should rather be
> `client.videoCodec =` for properties.
> 
> Also, consider making the property read-only and passing it in the
initializer.
> Seems safer to me since we make the assumption in ARDAppClient that it's
always
> set.

Done.

[kthelgason, 2017-03-10 13:28:01]

Looks great, lgtm!

[magjed_webrtc, 2017-03-15 14:10:17]

Description was changed from

==========
Add video codec setting to AppRTCMobile on iOS.

List items in settings menu no longer stay selected. Checkmarks are now
added to the selected options before the view appears.

BUG=webrtc:7316
==========

to

==========
Add video codec setting to AppRTCMobile on iOS.

List items in settings menu no longer stay selected. Checkmarks are now
added to the selected options before the view appears.

BUG=webrtc:7316
TBR=denicija
==========

[magjed_webrtc, 2017-03-15 14:10:27]

lgtm

I would like Daniela to review ARDSettingsViewController.m retrospectively once
she comes back.

[sakal, 2017-03-15 14:18:46]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2017-03-15 14:18:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-15 14:24:23]

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

[commit-bot: I haz the power, 2017-03-15 14:24:23]

Try jobs failed on following builders:
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/19864)

[sakal, 2017-03-15 15:13:04]

The CQ bit was checked by sakal@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 15:13:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-15 15:16:19]

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

[commit-bot: I haz the power, 2017-03-15 15:16:20]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)

[sakal, 2017-03-17 14:50:04]

The CQ bit was checked by sakal@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 14:50:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-17 15:14:06]

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

[commit-bot: I haz the power, 2017-03-17 15:14:07]

Dry run: This issue passed the CQ dry run.

[sakal, 2017-03-17 15:59:13]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2017-03-17 15:59:13]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
kthelgason@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2735303004/#ps140001 (title:
"Fix AppRTCMobile on Mac.")

[commit-bot: I haz the power, 2017-03-17 15:59:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-17 16:01:59]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489766353289740,
"parent_rev": "533aedc492c429ec4fcb2fdc4e4a7498d61b69b8", "commit_rev":
"68b5df97c77a1ae5e16ec27f83cbce098411da0e"}

[commit-bot: I haz the power, 2017-03-17 16:02:03]

Description was changed from

==========
Add video codec setting to AppRTCMobile on iOS.

List items in settings menu no longer stay selected. Checkmarks are now
added to the selected options before the view appears.

BUG=webrtc:7316
TBR=denicija
==========

to

==========
Add video codec setting to AppRTCMobile on iOS.

List items in settings menu no longer stay selected. Checkmarks are now
added to the selected options before the view appears.

BUG=webrtc:7316
TBR=denicija

Review-Url: https://codereview.webrtc.org/2735303004
Cr-Commit-Position: refs/heads/master@{#17296}
Committed:
https://chromium.googlesource.com/external/webrtc/+/68b5df97c77a1ae5e16ec27f8...
==========

[commit-bot: I haz the power, 2017-03-17 16:02:04]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/68b5df97c77a1ae5e16ec27f8...

[daniela-webrtc, 2017-03-20 13:10:05]

denicija@webrtc.org changed reviewers:
+ denicija@webrtc.org

[daniela-webrtc, 2017-03-20 13:10:07]

Great work Sami! This looks really good!
Some (mostly style) comments bellow, that can be addressed in some future CL.

https://codereview.webrtc.org/2735303004/diff/140001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ARDAppClient.h (right):

https://codereview.webrtc.org/2735303004/diff/140001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDAppClient.h:64:
preferVideoCodec:(NSString*)codec;
NSString* -> NSString *

https://codereview.webrtc.org/2735303004/diff/140001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDAppClient.h:64:
preferVideoCodec:(NSString*)codec;
I don't think this is needed as early as the init?
It would be nice to pass all settings the same way i.e either in the initializer
or set as parameters like the media constraints and the bitrate are currently
set.
Having parameters will enable reset of settings when the (ARDApp)client is
cached and not recreated all the time.

https://codereview.webrtc.org/2735303004/diff/140001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsModel.h (right):

https://codereview.webrtc.org/2735303004/diff/140001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsModel.h:57: * Returns current
video codec setting from store if present.
This documentation can be improved slightly. Suggestion:
"Returns current video codec setting from store if present or default (H264)
otherwise".

https://codereview.webrtc.org/2735303004/diff/140001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsModel.h:64: * If the provided
constraint is not part of the available video codecs
provided constraint -> provided video codec

https://codereview.webrtc.org/2735303004/diff/140001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.h (right):

https://codereview.webrtc.org/2735303004/diff/140001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsStore.h:22:
@property(nonatomic) NSString* videoCodec;
NSString* -> NSString *

https://codereview.webrtc.org/2735303004/diff/140001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m (right):

https://codereview.webrtc.org/2735303004/diff/140001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ios/ARDSettingsViewController.m:57: -
(void)viewDidAppear:(BOOL)animated {
This can be removed