WebRTC Code Reviews
|
|
DescriptionReplaced duplicating code with function call as a fix to https://webrtc-codereview.appspot.com/45439004/.
Patch Set 1 #
Total comments: 4
Patch Set 2 : Updated comment according to reviewer request. #Patch Set 3 : Split GetiOSProxySettings function into two for it to look more like mac functions and for easier u… #
Total comments: 4
Patch Set 4 : Fixed issues pointed by reviewer. #
Total comments: 4
Patch Set 5 : Fixed code styling as requested. #Messages
Total messages: 16 (5 generated)
juberti@google.com changed reviewers: + juberti@google.com
Sign in to reply to this message.
for a unit test, can you create a dictionary object in the test with the right keys, and pass that in?
Sign in to reply to this message.
pthatcher@webrtc.org changed reviewers: + pthatcher@webrtc.org
Sign in to reply to this message.
For unit tests, Justin is suggesting passing in a dictionary. A similar approach that would end up having more test coverage, would be to fake out CFNetworkCopySystemProxySettings. I'm not sure if it's easier to mock out CFNetworkCopySystemProxySettings or mock our what it returns, but either way would work. https://webrtc-codereview.appspot.com/55509004/diff/1/webrtc/base/proxydetect.cc File webrtc/base/proxydetect.cc (right): https://webrtc-codereview.appspot.com/55509004/diff/1/webrtc/base/proxydetect... webrtc/base/proxydetect.cc:1003: // proxy settings. Can you fix this comment while you're here? Perhaps like this: #if defined(WEBRTC_MAC) && !defined(WEBRTC_IOS) // Specific implementation for reading system wide proxy settings.
Sign in to reply to this message.
Regarding unit tests, I think it is much easier to test the function that takes a dictionary. I'd just do that (instead of mocking out factory function). See comment about functionality though. https://webrtc-codereview.appspot.com/55509004/diff/1/webrtc/base/proxydetect.cc File webrtc/base/proxydetect.cc (right): https://webrtc-codereview.appspot.com/55509004/diff/1/webrtc/base/proxydetect... webrtc/base/proxydetect.cc:1203: // Note that flute doesn't differentiate between HTTPS and HTTP, Is this right? Should we be calling GetMacProxySettingsFromDict instead? IOW, if there is a single proxy that does both HTTP and HTTPS, or separate proxies for HTTP and HTTPS, don't we need to be able to handle both cases? Or does iOS only support the single proxy case?
Sign in to reply to this message.
https://webrtc-codereview.appspot.com/55509004/diff/1/webrtc/base/proxydetect.cc File webrtc/base/proxydetect.cc (right): https://webrtc-codereview.appspot.com/55509004/diff/1/webrtc/base/proxydetect... webrtc/base/proxydetect.cc:1003: // proxy settings. On 2015/05/27 17:53:31, pthatcher wrote: > Can you fix this comment while you're here? Perhaps like this: > > #if defined(WEBRTC_MAC) && !defined(WEBRTC_IOS) > // Specific implementation for reading system wide proxy settings. Done. https://webrtc-codereview.appspot.com/55509004/diff/1/webrtc/base/proxydetect... webrtc/base/proxydetect.cc:1203: // Note that flute doesn't differentiate between HTTPS and HTTP, On 2015/05/28 23:28:37, juberti wrote: > Is this right? Should we be calling GetMacProxySettingsFromDict instead? It uses different dictionary keys which are mac specific so we can't use iit for iOS. > IOW, if there is a single proxy that does both HTTP and HTTPS, or separate > proxies for HTTP and HTTPS, don't we need to be able to handle both cases? > > Or does iOS only support the single proxy case? Yes, iOS in WiFi settings allows to setup only HTTP proxy.
Sign in to reply to this message.
youwrk@gmail.com changed reviewers: + tkchin@webrtc.org
Sign in to reply to this message.
On 2015/05/28 23:28:37, juberti wrote:
> Regarding unit tests, I think it is much easier to test the function that
takes
> a dictionary. I'd just do that (instead of mocking out factory function).
>
On 2015/05/28 23:28:37, juberti wrote:
> Regarding unit tests, I think it is much easier to test the function that
takes
> a dictionary. I'd just do that (instead of mocking out factory function).
Adding tkchin to reviewers and CC for iOS part.
Regarding unit tests. I have tried to implement one but:
1. try_bots for iOS do not run unit tests
2. At the moment AppRTCDemo works fine without CFNetwork framework dependency in
rtc_base because this framwork is linked somewhere else. For rtc_unittests
target I have to add it to rtc_base. I am not sure but it might be that I
actually have to add it anyway for this CL since it is used in proxydetect.cc
now.
I would appreciate some thought on this matter.
Unit test, I have made, looks like this:
#ifdef WEBRTC_IOS
bool GetiOSProxySettingsFromDictionary(ProxyInfo* proxy,
const CFDictionaryRef proxy_dict);
#endif
// Verifies that a HTTP proxy is detected if configured on iOS.
TEST_F(ProxyDetectTest, TestiOSProxyHttp) {
ProxyInfo proxy_info;
SocketAddress proxy_address("proxy.http.com", 8888);
CFMutableDictionaryRef proxy_dict =
CFDictionaryCreateMutable(NULL,
0,
&kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks);
CFStringRef host = CFStringCreateWithCString(NULL,
"proxy.http.com",
kCFStringEncodingASCII);
short portC = 8888;
CFNumberRef port = CFNumberCreate(NULL, kCFNumberShortType, &portC);
char yes = 1;
CFNumberRef enabled = CFNumberCreate(NULL, kCFNumberCharType, &yes);
CFDictionaryAddValue(proxy_dict, kCFNetworkProxiesHTTPProxy, host);
CFDictionaryAddValue(proxy_dict, kCFNetworkProxiesHTTPPort, port);
CFDictionaryAddValue(proxy_dict, kCFNetworkProxiesHTTPEnable, enabled);
CFRelease(host);
CFRelease(port);
CFRelease(enabled);
EXPECT_TRUE(GetiOSProxySettingsFromDictionary(&proxy_info, proxy_dict));
CFRelease(proxy_dict);
EXPECT_EQ(PROXY_HTTPS, proxy_info.type);
EXPECT_EQ(proxy_address, proxy_info.address);
}
Sign in to reply to this message.
https://webrtc-codereview.appspot.com/55509004/diff/40001/webrtc/base/proxyde... File webrtc/base/proxydetect.cc (right): https://webrtc-codereview.appspot.com/55509004/diff/40001/webrtc/base/proxyde... webrtc/base/proxydetect.cc:1203: bool gotProxy = false; style: got_proxy https://webrtc-codereview.appspot.com/55509004/diff/40001/webrtc/base/proxyde... webrtc/base/proxydetect.cc:1205: // Note that flute doesn't differentiate between HTTPS and HTTP, 'flute' is dead, so suggest rewording this as // We only care about HTTPS proxies, so read the HTTP proxy info as PROXY_HTTPS.
Sign in to reply to this message.
PTAL. + gentle reminder of question about unit test and framwork https://webrtc-codereview.appspot.com/55509004/diff/40001/webrtc/base/proxyde... File webrtc/base/proxydetect.cc (right): https://webrtc-codereview.appspot.com/55509004/diff/40001/webrtc/base/proxyde... webrtc/base/proxydetect.cc:1203: bool gotProxy = false; On 2015/05/29 21:51:37, juberti wrote: > style: got_proxy Done. https://webrtc-codereview.appspot.com/55509004/diff/40001/webrtc/base/proxyde... webrtc/base/proxydetect.cc:1205: // Note that flute doesn't differentiate between HTTPS and HTTP, On 2015/05/29 21:51:37, juberti wrote: > 'flute' is dead, so suggest rewording this as > > // We only care about HTTPS proxies, so read the HTTP proxy info as PROXY_HTTPS. Done.
Sign in to reply to this message.
juberti@google.com changed reviewers: + haysc@webrtc.org - tkchin@webrtc.org
Sign in to reply to this message.
code lgtm added chuck as reviewer for unit test (is there not a simpler way to create a dictionary?)
Sign in to reply to this message.
On 2015/06/02 04:55:47, juberti wrote:
> code lgtm
>
> added chuck as reviewer for unit test (is there not a simpler way to create a
> dictionary?)
Well, I have simplified it a bit and replaced short/char with arch independent
int16_t/int8_t.
This is not Foundation and Obj-C, so no syntax sugar.
TEST_F(ProxyDetectTest, TestiOSProxyHttp) {
ProxyInfo proxy_info;
SocketAddress proxy_address("proxy.http.com", 8888);
CFMutableDictionaryRef proxy_dict =
CFDictionaryCreateMutable(NULL,
0,
&kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks);
int16_t portC = 8888;
CFNumberRef port = CFNumberCreate(NULL, kCFNumberSInt16Type, &portC);
int8_t yes = 1;
CFNumberRef enabled = CFNumberCreate(NULL, kCFNumberSInt8Type, &yes);
CFDictionaryAddValue(proxy_dict,
kCFNetworkProxiesHTTPProxy,
CFSTR("proxy.http.com"));
CFDictionaryAddValue(proxy_dict, kCFNetworkProxiesHTTPPort, port);
CFDictionaryAddValue(proxy_dict, kCFNetworkProxiesHTTPEnable, enabled);
CFRelease(port);
CFRelease(enabled);
EXPECT_TRUE(GetiOSProxySettingsFromDictionary(&proxy_info, proxy_dict));
CFRelease(proxy_dict);
EXPECT_EQ(PROXY_HTTPS, proxy_info.type);
EXPECT_EQ(proxy_address, proxy_info.address);
}
Sign in to reply to this message.
tkchin@webrtc.org changed reviewers: + tkchin@webrtc.org
Sign in to reply to this message.
re: unit tests It's true that unit tests are not being run on bots at the moment. I would still add it and make sure that that particular test passes by hand using gtest_filter on your device manually. We will fix any breakages once we get rtc_unittests compiling and passing on bots. https://webrtc-codereview.appspot.com/55509004/diff/60001/webrtc/base/proxyde... File webrtc/base/proxydetect.cc (right): https://webrtc-codereview.appspot.com/55509004/diff/60001/webrtc/base/proxyde... webrtc/base/proxydetect.cc:1205: // We only care about HTTPS proxies, so read the HTTP proxy info as PROXY_HTTPS. nit: align comment. (2 spaces) https://webrtc-codereview.appspot.com/55509004/diff/60001/webrtc/base/proxyde... webrtc/base/proxydetect.cc:1207: PROXY_HTTPS, nit: the second argument onwards should have an extra space (align with first letter of first argument)
Sign in to reply to this message.
@tkchin re re: unit tests > make sure that that particular test passes by hand using gtest_filter on your device manually. How do I do that? https://webrtc-codereview.appspot.com/55509004/diff/60001/webrtc/base/proxyde... File webrtc/base/proxydetect.cc (right): https://webrtc-codereview.appspot.com/55509004/diff/60001/webrtc/base/proxyde... webrtc/base/proxydetect.cc:1205: // We only care about HTTPS proxies, so read the HTTP proxy info as PROXY_HTTPS. On 2015/06/08 21:24:24, tkchin wrote: > nit: align comment. (2 spaces) Done. https://webrtc-codereview.appspot.com/55509004/diff/60001/webrtc/base/proxyde... webrtc/base/proxydetect.cc:1207: PROXY_HTTPS, On 2015/06/08 21:24:24, tkchin wrote: > nit: the second argument onwards should have an extra space (align with first > letter of first argument) Done.
Sign in to reply to this message.
|
