WebRTC Code Reviews
Help | Chromium Project | Sign in
(1270)

Issue 55509004: Replaced duplicating code with function call as a fix to https://webrtc-codereview.appspot.com/4543…

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 11 months ago by youwrk
Modified:
3 years, 10 months ago
Reviewers:
juberti, tkchin, pthatcher, Chuck
CC:
webrtc-reviews_webrtc.org, tterriberry, joachim, tkchin
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Replaced 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -31 lines) Patch
M webrtc/base/proxydetect.cc View 1 2 3 4 4 chunks +30 lines, -31 lines 0 comments Download
Trybot results: Sign in to try more bots
Project "webrtc" does not have a commit queue.

Messages

Total messages: 16 (5 generated)
juberti
for a unit test, can you create a dictionary object in the test with the ...
3 years, 11 months ago (2015-05-27 17:00:05 UTC) #2
pthatcher
For unit tests, Justin is suggesting passing in a dictionary. A similar approach that would ...
3 years, 11 months ago (2015-05-27 17:53:31 UTC) #4
juberti
Regarding unit tests, I think it is much easier to test the function that takes ...
3 years, 11 months ago (2015-05-28 23:28:37 UTC) #5
youwrk
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.cc#newcode1003 webrtc/base/proxydetect.cc:1003: // proxy settings. On 2015/05/27 17:53:31, pthatcher wrote: > ...
3 years, 11 months ago (2015-05-29 10:20:58 UTC) #6
youwrk
On 2015/05/28 23:28:37, juberti wrote: > Regarding unit tests, I think it is much easier ...
3 years, 11 months ago (2015-05-29 15:06:52 UTC) #8
juberti
https://webrtc-codereview.appspot.com/55509004/diff/40001/webrtc/base/proxydetect.cc File webrtc/base/proxydetect.cc (right): https://webrtc-codereview.appspot.com/55509004/diff/40001/webrtc/base/proxydetect.cc#newcode1203 webrtc/base/proxydetect.cc:1203: bool gotProxy = false; style: got_proxy https://webrtc-codereview.appspot.com/55509004/diff/40001/webrtc/base/proxydetect.cc#newcode1205 webrtc/base/proxydetect.cc:1205: // ...
3 years, 11 months ago (2015-05-29 21:51:37 UTC) #9
youwrk
PTAL. + gentle reminder of question about unit test and framwork https://webrtc-codereview.appspot.com/55509004/diff/40001/webrtc/base/proxydetect.cc File webrtc/base/proxydetect.cc (right): ...
3 years, 10 months ago (2015-06-01 13:55:58 UTC) #10
juberti
code lgtm added chuck as reviewer for unit test (is there not a simpler way ...
3 years, 10 months ago (2015-06-02 04:55:47 UTC) #12
youwrk
On 2015/06/02 04:55:47, juberti wrote: > code lgtm > > added chuck as reviewer for ...
3 years, 10 months ago (2015-06-02 07:37:47 UTC) #13
tkchin
re: unit tests It's true that unit tests are not being run on bots at ...
3 years, 10 months ago (2015-06-08 21:24:24 UTC) #15
youwrk
3 years, 10 months ago (2015-06-15 08:11:41 UTC) #16
@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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 245c2c2-tainted