Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2465733002:
Remove stl_util's deletion function use from components/invalidation/. (Closed)
|
Created:
4 years, 1 month ago by Avi (use Gerrit) Modified:
4 years, 1 month ago Reviewers:
dcheng CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove stl_util's deletion function use from components/invalidation/.
BUG=555865
Committed: https://crrev.com/89df5e0546823b3cfd7721343bf71a77b2f60d9d
Cr-Commit-Position: refs/heads/master@{#428714}
Patch Set 1 #
Total comments: 8
Patch Set 2 : nits #
Messages
Total messages: 21 (10 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...
Description was changed from ========== Remove stl_util's deletion function use from components/invalidation/. BUG=555865, 659145 ========== to ========== Remove stl_util's deletion function use from components/invalidation/. BUG=555865 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
avi@chromium.org changed reviewers: + dcheng@chromium.org
LGTM https://codereview.chromium.org/2465733002/diff/1/components/invalidation/imp... File components/invalidation/impl/registration_manager.cc (right): https://codereview.chromium.org/2465733002/diff/1/components/invalidation/imp... components/invalidation/impl/registration_manager.cc:166: for (auto it = registration_statuses_.begin(); Optional nit: for-each loop? https://codereview.chromium.org/2465733002/diff/1/components/invalidation/imp... components/invalidation/impl/registration_manager.cc:185: for (auto it = registration_statuses_.begin(); Optional nit ditto. https://codereview.chromium.org/2465733002/diff/1/components/invalidation/imp... components/invalidation/impl/registration_manager.cc:284: for (auto it = registration_statuses_.begin(); Ditto with for-each here. https://codereview.chromium.org/2465733002/diff/1/components/invalidation/imp... File components/invalidation/impl/sync_system_resources.cc (right): https://codereview.chromium.org/2465733002/diff/1/components/invalidation/imp... components/invalidation/impl/sync_system_resources.cc:134: std::find_if(posted_tasks_.begin(), posted_tasks_.end(), Part of me almost feels like we should just have a utility for this in stl_util.h...
The CQ bit was checked by avi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2465733002/#ps20001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2465733002/diff/1/components/invalidation/imp... File components/invalidation/impl/registration_manager.cc (right): https://codereview.chromium.org/2465733002/diff/1/components/invalidation/imp... components/invalidation/impl/registration_manager.cc:166: for (auto it = registration_statuses_.begin(); On 2016/10/31 00:19:29, dcheng wrote: > Optional nit: for-each loop? Done. https://codereview.chromium.org/2465733002/diff/1/components/invalidation/imp... components/invalidation/impl/registration_manager.cc:185: for (auto it = registration_statuses_.begin(); On 2016/10/31 00:19:29, dcheng wrote: > Optional nit ditto. Done. https://codereview.chromium.org/2465733002/diff/1/components/invalidation/imp... components/invalidation/impl/registration_manager.cc:284: for (auto it = registration_statuses_.begin(); On 2016/10/31 00:19:29, dcheng wrote: > Ditto with for-each here. Done. https://codereview.chromium.org/2465733002/diff/1/components/invalidation/imp... File components/invalidation/impl/sync_system_resources.cc (right): https://codereview.chromium.org/2465733002/diff/1/components/invalidation/imp... components/invalidation/impl/sync_system_resources.cc:134: std::find_if(posted_tasks_.begin(), posted_tasks_.end(), On 2016/10/31 00:19:29, dcheng wrote: > Part of me almost feels like we should just have a utility for this in > stl_util.h... Acknowledged.
Message was sent while issue was closed.
Description was changed from ========== Remove stl_util's deletion function use from components/invalidation/. BUG=555865 ========== to ========== Remove stl_util's deletion function use from components/invalidation/. BUG=555865 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove stl_util's deletion function use from components/invalidation/. BUG=555865 ========== to ========== Remove stl_util's deletion function use from components/invalidation/. BUG=555865 Committed: https://crrev.com/89df5e0546823b3cfd7721343bf71a77b2f60d9d Cr-Commit-Position: refs/heads/master@{#428714} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/89df5e0546823b3cfd7721343bf71a77b2f60d9d Cr-Commit-Position: refs/heads/master@{#428714}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2464903003/ by xidachen@chromium.org. The reason for reverting is: suspect causing failure here: https://build.chromium.org/p/chromium.mac/builders/ios-simulator/builds/6991.
Message was sent while issue was closed.
On 2016/10/31 16:52:28, xidachen wrote: > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/2464903003/ by mailto:xidachen@chromium.org. > > The reason for reverting is: suspect causing failure here: > https://build.chromium.org/p/chromium.mac/builders/ios-simulator/builds/6991. Sorry, I reverted the wrong CL. Your CL is not the cause of the failure. I apologize for that, I will revert my revert.
Message was sent while issue was closed.
On 2016/10/31 17:07:12, xidachen wrote: > On 2016/10/31 16:52:28, xidachen wrote: > > A revert of this CL (patchset #2 id:20001) has been created in > > https://codereview.chromium.org/2464903003/ by mailto:xidachen@chromium.org. > > > > The reason for reverting is: suspect causing failure here: > > https://build.chromium.org/p/chromium.mac/builders/ios-simulator/builds/6991. > > Sorry, I reverted the wrong CL. Your CL is not the cause of the failure. I > apologize for that, I will revert my revert. No worries! Please never feel like you must apologize for reverting me, even speculatively.
Message was sent while issue was closed.
On 2016/10/31 at 18:22:06, avi wrote: > On 2016/10/31 17:07:12, xidachen wrote: > > On 2016/10/31 16:52:28, xidachen wrote: > > > A revert of this CL (patchset #2 id:20001) has been created in > > > https://codereview.chromium.org/2464903003/ by mailto:xidachen@chromium.org. > > > > > > The reason for reverting is: suspect causing failure here: > > > https://build.chromium.org/p/chromium.mac/builders/ios-simulator/builds/6991. > > > > Sorry, I reverted the wrong CL. Your CL is not the cause of the failure. I > > apologize for that, I will revert my revert. > > No worries! Please never feel like you must apologize for reverting me, even speculatively. Are these invalidations the same as paint invalidations? Have some paint invalidation tests which are flaky and this CL is in range: https://sheriff-o-matic.appspot.com/chromium/examine/chromium.webkit.WebKit%2....
Message was sent while issue was closed.
On 2016/10/31 23:13:14, DaleCurtis wrote: > On 2016/10/31 at 18:22:06, avi wrote: > > On 2016/10/31 17:07:12, xidachen wrote: > > > On 2016/10/31 16:52:28, xidachen wrote: > > > > A revert of this CL (patchset #2 id:20001) has been created in > > > > https://codereview.chromium.org/2464903003/ by > mailto:xidachen@chromium.org. > > > > > > > > The reason for reverting is: suspect causing failure here: > > > > > https://build.chromium.org/p/chromium.mac/builders/ios-simulator/builds/6991. > > > > > > Sorry, I reverted the wrong CL. Your CL is not the cause of the failure. I > > > apologize for that, I will revert my revert. > > > > No worries! Please never feel like you must apologize for reverting me, even > speculatively. > > Are these invalidations the same as paint invalidations? Have some paint > invalidation tests which are flaky and this CL is in range: > > https://sheriff-o-matic.appspot.com/chromium/examine/chromium.webkit.WebKit%2.... No, they are two different things. components/invalidation is the interface between chrome code and third_party/cacheinvalidation (which is the backend for sync and various other things) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
