Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1286373002:
Refactored not to expose raw pointers on ProxyList class. (Closed)
DescriptionRefactored not to expose raw pointers on ProxyList class.
BUG=472381
R=eroman@chromium.org
Committed: https://crrev.com/4e6518f3f586a45e559bee72ec1b6a41ab0d8988
Cr-Commit-Position: refs/heads/master@{#345237}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #
Messages
Total messages: 25 (12 generated)
Refactored not to expose raw pointers on ProxyList class. unittested with Proxy* test-cases. Please kindly review :)
https://codereview.chromium.org/1286373002/diff/1/net/proxy/proxy_list.h File net/proxy/proxy_list.h (right): https://codereview.chromium.org/1286373002/diff/1/net/proxy/proxy_list.h#newc... net/proxy/proxy_list.h:81: // Returns a serialized value for the list. The caller takes ownership of it. Remove comment about ownership since that is now implied by the API https://codereview.chromium.org/1286373002/diff/1/net/proxy/proxy_list.h#newc... net/proxy/proxy_list.h:90: bool Fallback(ProxyRetryInfoMap& proxy_retry_info, This violates the style guide: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar... For parameters that are mutated we use pointers https://codereview.chromium.org/1286373002/diff/1/net/proxy/proxy_list.h#newc... net/proxy/proxy_list.h:117: void AddProxyToRetryList(ProxyRetryInfoMap& proxy_retry_info, Same comment as earlier
On 2015/08/18 01:32:10, eroman wrote: > https://codereview.chromium.org/1286373002/diff/1/net/proxy/proxy_list.h > File net/proxy/proxy_list.h (right): > > https://codereview.chromium.org/1286373002/diff/1/net/proxy/proxy_list.h#newc... > net/proxy/proxy_list.h:81: // Returns a serialized value for the list. The > caller takes ownership of it. > Remove comment about ownership since that is now implied by the API > > https://codereview.chromium.org/1286373002/diff/1/net/proxy/proxy_list.h#newc... > net/proxy/proxy_list.h:90: bool Fallback(ProxyRetryInfoMap& proxy_retry_info, > This violates the style guide: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar... > > For parameters that are mutated we use pointers > > https://codereview.chromium.org/1286373002/diff/1/net/proxy/proxy_list.h#newc... > net/proxy/proxy_list.h:117: void AddProxyToRetryList(ProxyRetryInfoMap& > proxy_retry_info, > Same comment as earlier Thank you for your valuable review. Please look this patchset.
LGTM. (You are not in the AUTHORS file yet, you can either add it to this changelist, or submit one of the other changes first which includes AUTHORS).
Thank you for your guide about AUTHORS file. I updated AUTHORS file in this patch. PTAL https://codereview.chromium.org/1286373002/diff/1/net/proxy/proxy_list.h File net/proxy/proxy_list.h (right): https://codereview.chromium.org/1286373002/diff/1/net/proxy/proxy_list.h#newc... net/proxy/proxy_list.h:81: // Returns a serialized value for the list. The caller takes ownership of it. On 2015/08/18 01:32:09, eroman wrote: > Remove comment about ownership since that is now implied by the API Done. https://codereview.chromium.org/1286373002/diff/1/net/proxy/proxy_list.h#newc... net/proxy/proxy_list.h:90: bool Fallback(ProxyRetryInfoMap& proxy_retry_info, On 2015/08/18 01:32:10, eroman wrote: > This violates the style guide: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar... > > For parameters that are mutated we use pointers I think it's better just to use raw pointers in here. https://codereview.chromium.org/1286373002/diff/1/net/proxy/proxy_list.h#newc... net/proxy/proxy_list.h:117: void AddProxyToRetryList(ProxyRetryInfoMap& proxy_retry_info, On 2015/08/18 01:32:09, eroman wrote: > Same comment as earlier ditto
lgtm
The CQ bit was checked by msu.koo@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1286373002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1286373002/40001
The CQ bit was unchecked by msu.koo@samsung.com
The CQ bit was checked by msu.koo@samsung.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by msu.koo@samsung.com
The CQ bit was unchecked by msu.koo@samsung.com
The CQ bit was unchecked by msu.koo@samsung.com
The CQ bit was checked by msu.koo@samsung.com to run a CQ dry run
The CQ bit was unchecked by msu.koo@samsung.com
The CQ bit was checked by msu.koo@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1286373002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1286373002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msu.koo@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1286373002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1286373002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4e6518f3f586a45e559bee72ec1b6a41ab0d8988 Cr-Commit-Position: refs/heads/master@{#345237} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
