|
|
Created:
4 years, 4 months ago by Avi (use Gerrit) Modified:
4 years, 4 months ago CC:
chromium-reviews, groby+spellwatch_chromium.org, grt+watch_chromium.org, rlp+watch_chromium.org, rouslan+spell_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove stl_util's STLDeleteContainerPairPointers.
Along the way, remove some linked_ptr usage, too.
BUG=555865, 556939
Committed: https://crrev.com/d99bd1a3a659975bc78b4b2dcb900537c0ef761b
Cr-Commit-Position: refs/heads/master@{#412674}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #
Total comments: 6
Patch Set 4 : mattm #Patch Set 5 : rebase #Patch Set 6 : rebase for reals #Patch Set 7 : mattm #
Messages
Total messages: 47 (34 generated)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove stl_util's STLDeleteContainerPairPointers. Along the way, remove some linked_ptr usage, too. BUG=555865,556939 ========== to ========== Remove stl_util's STLDeleteContainerPairPointers. Along the way, remove some linked_ptr usage, too. BUG=555865,556939 ==========
avi@chromium.org changed reviewers: + groby@chromium.org, mark@chromium.org, mattm@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
groby: spellchecker mattm: safe browsing mark: base
On 2016/08/13 04:45:29, Avi wrote: > groby: spellchecker > mattm: safe browsing > mark: base Ping?
LGTM in base
Nice cleanup, just a few nits. https://codereview.chromium.org/2240083004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/client_side_detection_service.cc (right): https://codereview.chromium.org/2240083004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/client_side_detection_service.cc:288: net::URLFetcher* fetcher = Would be cleaner to store the temporaries as unique_ptrs instead of bare pointers that later get wrapped. https://codereview.chromium.org/2240083004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc (right): https://codereview.chromium.org/2240083004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:158: csd_service_->cache_[gurl] = base::WrapUnique( Use MakeUnique instead of WrapUnique? (see chromium-dev thread) (applies elsewhere too)
Neat. LGTM, and a question just for my own curiosity: Why not make the callback data move-only and get rid of a bunch of ptr handling for that?
On 2016/08/17 16:34:17, groby wrote: > Neat. > > LGTM, and a question just for my own curiosity: Why not make the callback data > move-only and get rid of a bunch of ptr handling for that? Mostly because getting rid of the stl_util functions are a side project for me, and I'm not very familiar with a lot of the code in which I'm working. I'm trying to not make CLs that are obviously sub-optimal, but I'm trying to not go crazy and rewrite everyone else's code, either.
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2240083004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/client_side_detection_service.cc (right): https://codereview.chromium.org/2240083004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/client_side_detection_service.cc:288: net::URLFetcher* fetcher = On 2016/08/16 20:41:22, mattm wrote: > Would be cleaner to store the temporaries as unique_ptrs instead of bare > pointers that later get wrapped. Hmmm. Once we move them, then we can't use the temporaries. I'll rewrite but I'm not sure how much cleaner it is. https://codereview.chromium.org/2240083004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc (right): https://codereview.chromium.org/2240083004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:158: csd_service_->cache_[gurl] = base::WrapUnique( On 2016/08/16 20:41:22, mattm wrote: > Use MakeUnique instead of WrapUnique? (see chromium-dev thread) (applies > elsewhere too) Sometimes MakeUnique works, sometimes it fails for access reasons or Create() functions. I'm unfamiliar with a lot of this code, and sometimes I use WrapUnique just because it's the most minimally-invasive way to do the conversion.
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mattm, ptal.
lgtm https://codereview.chromium.org/2240083004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc (right): https://codereview.chromium.org/2240083004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:158: csd_service_->cache_[gurl] = base::WrapUnique( On 2016/08/17 18:25:37, Avi wrote: > On 2016/08/16 20:41:22, mattm wrote: > > Use MakeUnique instead of WrapUnique? (see chromium-dev thread) (applies > > elsewhere too) > > Sometimes MakeUnique works, sometimes it fails for access reasons or Create() > functions. I'm unfamiliar with a lot of this code, and sometimes I use > WrapUnique just because it's the most minimally-invasive way to do the > conversion. nit: Could you update the other ones in here too?
https://codereview.chromium.org/2240083004/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/client_side_detection_service_unittest.cc (right): https://codereview.chromium.org/2240083004/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/client_side_detection_service_unittest.cc:158: csd_service_->cache_[gurl] = base::WrapUnique( On 2016/08/17 21:28:34, mattm wrote: > On 2016/08/17 18:25:37, Avi wrote: > > On 2016/08/16 20:41:22, mattm wrote: > > > Use MakeUnique instead of WrapUnique? (see chromium-dev thread) (applies > > > elsewhere too) > > > > Sometimes MakeUnique works, sometimes it fails for access reasons or Create() > > functions. I'm unfamiliar with a lot of this code, and sometimes I use > > WrapUnique just because it's the most minimally-invasive way to do the > > conversion. > > nit: Could you update the other ones in here too? Done.
The CQ bit was checked by avi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, groby@chromium.org, mattm@chromium.org Link to the patchset: https://codereview.chromium.org/2240083004/#ps120001 (title: "mattm")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove stl_util's STLDeleteContainerPairPointers. Along the way, remove some linked_ptr usage, too. BUG=555865,556939 ========== to ========== Remove stl_util's STLDeleteContainerPairPointers. Along the way, remove some linked_ptr usage, too. BUG=555865,556939 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Remove stl_util's STLDeleteContainerPairPointers. Along the way, remove some linked_ptr usage, too. BUG=555865,556939 ========== to ========== Remove stl_util's STLDeleteContainerPairPointers. Along the way, remove some linked_ptr usage, too. BUG=555865,556939 Committed: https://crrev.com/d99bd1a3a659975bc78b4b2dcb900537c0ef761b Cr-Commit-Position: refs/heads/master@{#412674} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d99bd1a3a659975bc78b4b2dcb900537c0ef761b Cr-Commit-Position: refs/heads/master@{#412674} |