EricssonResearch / openwebrtc-ios-sdk

SDK for adding OpenWebRTC to your iOS apps using CocoaPods
BSD 2-Clause "Simplified" License
69 stars 37 forks source link

add unregister functions for selfview / remoteview #42

Closed wangchauyan closed 8 years ago

wangchauyan commented 8 years ago

I saw the commit log (da1015e65e6f52a5df589143268a6a4d9bc2185a) which had removed the owr_window_registry_unregister (selfview/remoteview) from code.

But actually, we do need there two unregister functions. If we don't unregister that. We will get following warning message and then it will cause App crash.

** (<unknown>:5655): WARNING **: Tag 'self-view' has already been registered for another window handle

I think we should provide two unregister functions for developers for flexible using.

Test Environment :

stefanalund commented 8 years ago

I agree that this adds needed flexibility. What do you think about changing so that

[nativeHandler setSelfView:nil];

would do the same thing?

wangchauyan commented 8 years ago

I don't think setting nil would help. Actually I tried set nil before I traced the source code. It encountered the worst thing and then crashed. like :

[CALayer drawableProperties]: unrecognized selector sent to instance 0x1478c8ed0
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[CALayer drawableProperties]: unrecognized selector sent to instance 0x1478c8ed0'[CALayer drawableProperties]: unrecognized selector sent to instance 0x1478c8ed0
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[CALayer drawableProperties]: unrecognized selector sent to instance 0x1478c8ed0'

That's because we register the tag "SELF_VIEW_TAG / REMOTE_VIEW_TAG" again. I strongly recommend do unregister instead of setting nil.

In owr_window_registry.c, re-register the same tag would not help to release the old one. Should remove tag / handler from registry_hash_table. Could check these two functions

do_register

do_unregister

Thanks.

stefanalund commented 8 years ago

Let me clarify what I meant: Instead of adding new interface methods, my idea was to run your code if nil was sent in as the argument. Now that I think about it, this may be a bad idea since it is quite unintuitive. Let's go with your solution!

However, I noticed that there are differences in coding style. Would it be possible to correct them so that the codebase is uniform?

wangchauyan commented 8 years ago

Oh, I got your point, but I agree with you. I recommend to add new APIs instead of sending nil parameter to original function and do removing action.

Ok, I reformat the code style, please help to check that. Thanks a lot.

stefanalund commented 8 years ago

Looks good now, thanks!