Adds support for webrtc::test::ResourcePath on iOS

Adds support for webrtc::test::ResourcePath on iOS

BUG=webrtc:4752
R=tkchin@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/1d34fe979c52e5826c5c8162759b0167b2607836

[henrika_webrtc, 2015-06-11 08:18:36]

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

[henrika_webrtc, 2015-06-11 10:50:32]

https://codereview.chromium.org/1178843002/diff/20001/webrtc/test/testsupport...
File webrtc/test/testsupport/iosfileutils.mm (right):

https://codereview.chromium.org/1178843002/diff/20001/webrtc/test/testsupport...
webrtc/test/testsupport/iosfileutils.mm:24: NSString* nameString =
Could you please comment on the memory handling details here. Must I release
these? Not sure on how ARC is enabled. Thanks.

[henrika_webrtc, 2015-06-11 11:02:34]

https://codereview.chromium.org/1178843002/diff/20001/webrtc/test/testsupport...
File webrtc/test/testsupport/fileutils.cc (right):

https://codereview.chromium.org/1178843002/diff/20001/webrtc/test/testsupport...
webrtc/test/testsupport/fileutils.cc:214: return IOSResourcePath(name,
extension);
I should add comments here about what's unique for iOS. File is placed in root
of app bundle etc.

[henrika_webrtc, 2015-06-11 12:27:56]

https://codereview.chromium.org/1178843002/diff/20001/webrtc/test/testsupport...
File webrtc/test/testsupport/iosfileutils.mm (right):

https://codereview.chromium.org/1178843002/diff/20001/webrtc/test/testsupport...
webrtc/test/testsupport/iosfileutils.mm:24: NSString* nameString =
Perhaps I should do:

[[[NSString alloc] initWithUTF8String:file_name.c_str()] autorelease];

instead

[henrika_webrtc, 2015-06-11 13:10:59]

https://codereview.chromium.org/1178843002/diff/20001/webrtc/test/testsupport...
File webrtc/test/testsupport/iosfileutils.mm (right):

https://codereview.chromium.org/1178843002/diff/20001/webrtc/test/testsupport...
webrtc/test/testsupport/iosfileutils.mm:24: NSString* nameString =
sorry, or perhaps use the factory method stringWithUTF8String instead,

NSString* str = [NSString stringWithUTF8String:file_name.c_str()]

[tkchin_webrtc, 2015-06-12 01:30:26]

Very quick comments - will take another look tomorrow, didn't get a chance
today.

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/fileutils.cc (right):

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/f...
webrtc/test/testsupport/fileutils.cc:214: return IOSResourcePath(name,
extension);
On 2015/06/11 at 11:02:34, henrika_webrtc wrote:
> I should add comments here about what's unique for iOS. File is placed in root
of app bundle etc.

Probably add it to header file function comments.
Will this cause compiler warning that rest of code is unreachable?

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/i...
File webrtc/test/testsupport/iosfileutils.mm (right):

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/i...
webrtc/test/testsupport/iosfileutils.mm:24: NSString* nameString =
On 2015/06/11 at 10:50:31, henrika_webrtc wrote:
> Could you please comment on the memory handling details here. Must I release
these? Not sure on how ARC is enabled. Thanks.

You should enable ARC in GYP file. Use:
            'xcode_settings': {
              'CLANG_ENABLE_OBJC_ARC': 'YES',
            },

It may be worth guarding the file from non-ARC use:
#if !defined(__has_feature) || !__has_feature(objc_arc)
#error "This file requires ARC support."
#endif

Under ARC you never call retain/release/autorelease on objects. However, in here
you will need an autoreleasepool since there is no pool created in the test main
function. You can either add the autoreleasepool in the main function for the
test binary, or just here like follows:

@autoreleasepool {
   NSString* ....
}

See this for proper conversions between NSString* and std::string
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

[henrika_webrtc, 2015-06-12 12:52:11]

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/fileutils.cc (right):

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/f...
webrtc/test/testsupport/fileutils.cc:214: return IOSResourcePath(name,
extension);
I followed the existing style in unixfilesystem.cc in webrtc/base where an
mm-file is used without header. There are now compile warnings as is. Will add
comments in mm file. Not sure how to mix include and import.

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/i...
File webrtc/test/testsupport/iosfileutils.mm (right):

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/i...
webrtc/test/testsupport/iosfileutils.mm:24: NSString* nameString =
Thanks. Done changes but might have missed some details.

It is not clear to me why your StdStringFromNSString is better than the existing
pathString.UTF8String. Care to elaborate.

[henrika_webrtc, 2015-06-12 14:37:51]

I have now re enabled as many tests I can in the modules_unittest suite. Don't
think it is a good investment of time to enable all at this stage. Most of the
disabled tests are also disabled on Android and extensive work (by module
experts) is required to enable all of them.

PTAL

[tkchin_webrtc, 2015-06-12 17:43:35]

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/fileutils.cc (right):

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/f...
webrtc/test/testsupport/fileutils.cc:214: return IOSResourcePath(name,
extension);
On 2015/06/12 at 12:52:11, henrika_webrtc wrote:
> I followed the existing style in unixfilesystem.cc in webrtc/base where an
mm-file is used without header. There are now compile warnings as is. Will add
comments in mm file. Not sure how to mix include and import.

What compiler warnings are you referring to? I meant that because rest of code
isn't ifdef-ed out, you get something that looks like

  return ...;
  std::string resources_path = ...;
  ...
  return resources_path + name + "." + extension.

The style of private extern makes sense to me. I would namespace the function
under webrtc::test though.

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/i...
File webrtc/test/testsupport/iosfileutils.mm (right):

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/i...
webrtc/test/testsupport/iosfileutils.mm:24: NSString* nameString =
On 2015/06/12 at 12:52:11, henrika_webrtc wrote:
> Thanks. Done changes but might have missed some details.
> 
> It is not clear to me why your StdStringFromNSString is better than the
existing pathString.UTF8String. Care to elaborate.

In this case it doesn't matter. StdStringFromNSString does a copy, whereas
pathString.UTF8String does not, and accessing its bytes after NSString* is
dealloc-ed will cause issues.

https://codereview.webrtc.org/1178843002/diff/80001/webrtc/test/testsupport/i...
File webrtc/test/testsupport/iosfileutils.mm (right):

https://codereview.webrtc.org/1178843002/diff/80001/webrtc/test/testsupport/i...
webrtc/test/testsupport/iosfileutils.mm:23: NSString*
NSStringFromStdString(const std::string& stdString) {
We haven't been consistent with placement of * in mixed ObjC/C++ code. Usually
if just ObjC we want NSString *NSStringFromStdString (widely adopted style). I'm
okay with what you have for mixed code though.

Can you file bug / add TODO for placing these general functions in a shared
location?

https://codereview.webrtc.org/1178843002/diff/80001/webrtc/test/testsupport/i...
webrtc/test/testsupport/iosfileutils.mm:41: std::string file_name = name;
I think it is better to avoid mixing if all all possible. The ObjC way to do
this is:
NSString *path = NSStringFromStdString(name);
NSString *fileName = path.lastPathComponent;

https://codereview.webrtc.org/1178843002/diff/80001/webrtc/test/testsupport/i...
webrtc/test/testsupport/iosfileutils.mm:52: NSString* pathString = [[NSBundle
mainBundle] pathForResource: nameString
remove spaces after :

[henrika_webrtc, 2015-06-15 08:46:30]

Many thanks. PTAL.

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/fileutils.cc (right):

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/f...
webrtc/test/testsupport/fileutils.cc:214: return IOSResourcePath(name,
extension);
Bad spelling in first version, I did *not* get any compile warnings. But see
your point. Did changes.

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/i...
File webrtc/test/testsupport/iosfileutils.mm (right):

https://codereview.webrtc.org/1178843002/diff/20001/webrtc/test/testsupport/i...
webrtc/test/testsupport/iosfileutils.mm:24: NSString* nameString =
Thanks. Have shifted to using your methods.

https://codereview.webrtc.org/1178843002/diff/80001/webrtc/test/testsupport/i...
File webrtc/test/testsupport/iosfileutils.mm (right):

https://codereview.webrtc.org/1178843002/diff/80001/webrtc/test/testsupport/i...
webrtc/test/testsupport/iosfileutils.mm:23: NSString*
NSStringFromStdString(const std::string& stdString) {
Filed https://code.google.com/p/webrtc/issues/detail?id=4773 and added TODO.
Please add comments if my summary is not inline with what you intended.

PS; what do you mean by "with placement of * in mixed ObjC/C++ code" in this
context?

https://codereview.webrtc.org/1178843002/diff/80001/webrtc/test/testsupport/i...
webrtc/test/testsupport/iosfileutils.mm:41: std::string file_name = name;
Great stuff. I am no Objective-C guru...yet ;-)

https://codereview.webrtc.org/1178843002/diff/80001/webrtc/test/testsupport/i...
webrtc/test/testsupport/iosfileutils.mm:52: NSString* pathString = [[NSBundle
mainBundle] pathForResource: nameString
On 2015/06/12 17:43:35, tkchin_webrtc wrote:
> remove spaces after :

Done.

[tkchin_webrtc, 2015-06-15 20:23:14]

lgtm

https://codereview.webrtc.org/1178843002/diff/80001/webrtc/test/testsupport/i...
File webrtc/test/testsupport/iosfileutils.mm (right):

https://codereview.webrtc.org/1178843002/diff/80001/webrtc/test/testsupport/i...
webrtc/test/testsupport/iosfileutils.mm:23: NSString*
NSStringFromStdString(const std::string& stdString) {
On 2015/06/15 at 08:46:29, henrika_webrtc wrote:
> Filed https://code.google.com/p/webrtc/issues/detail?id=4773 and added TODO.
Please add comments if my summary is not inline with what you intended.
> 
> PS; what do you mean by "with placement of * in mixed ObjC/C++ code" in this
context?

Here we're defining static functions, but making use of ObjC classes. In C++,
the style is to place * next to type. eg. char* c; In ObjC the style is to place
* away from type, e.g. NSString *path; In mixed code, where std::string lives
together with NSString, it becomes a little bit murky, and I think it is
acceptable to use NSString* path in this case.

https://codereview.webrtc.org/1178843002/diff/120001/webrtc/test/testsupport/...
File webrtc/test/testsupport/iosfileutils.mm (right):

https://codereview.webrtc.org/1178843002/diff/120001/webrtc/test/testsupport/...
webrtc/test/testsupport/iosfileutils.mm:2: *  Copyright 2014 The WebRTC Project
Authors. All rights reserved.
2015

https://codereview.webrtc.org/1178843002/diff/120001/webrtc/test/testsupport/...
webrtc/test/testsupport/iosfileutils.mm:47: NSString *path =
NSStringFromStdString(name);
NSString* path =
NSString* fileName =
(sorry about misleading comment :P)

[tkchin_webrtc, 2015-06-15 20:23:17]

lgtm

[henrika_webrtc, 2015-06-16 08:04:31]

Committed patchset #8 (id:140001) manually as
1d34fe979c52e5826c5c8162759b0167b2607836 (presubmit successful).