Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1825823002:
Cleanup: LFH only needs to be enabled on XP/2003 (Closed)
|
Created:
4 years, 9 months ago by manzagop (departed) Modified:
4 years, 8 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCleanup: LFH only needs to be enabled on XP/2003
BUG=579196
Committed: https://crrev.com/aa3cff336c91fbe2cd8cc6254736564d95c5b230
Cr-Commit-Position: refs/heads/master@{#383983}
Patch Set 1 #Patch Set 2 : A few more instances #
Total comments: 2
Patch Set 3 : Address Scott's comment #Patch Set 4 : Merge #
Total comments: 2
Messages
Total messages: 32 (12 generated)
manzagop@chromium.org changed reviewers: + grt@chromium.org
Hey grt! This doesn't seem to be needed anymore. Please have a look! Thanks,
grt@chromium.org changed reviewers: + scottmg@chromium.org
Hi Scott. Are we in the clear to delete XP code? Should we use 579196 for random CLs like this?
Chrome no longer launches on XP and Vista so it's OK to start cleaning up. If it's something very invasive (not sure what that might be) consider waiting until 50 is stable in ~3w https://www.chromium.org/developers/calendar . For smaller/standalone deletes, go for it. BUG=579196 seems fine to me.
Maybe delete this one https://code.google.com/p/chromium/codesearch#chromium/src/base/allocator/all... at the same time.
Description was changed from ========== Cleanup: LFH only needs to be enabled on XP/2003 BUG= ========== to ========== Cleanup: LFH only needs to be enabled on XP/2003 BUG=579196 ==========
Added two other cases. Please have another look.
non-owner lgtm https://codereview.chromium.org/1825823002/diff/20001/base/allocator/allocato... File base/allocator/allocator_shim_win.cc (right): https://codereview.chromium.org/1825823002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_win.cc:47: return _crtheap != NULL; NULL -> nullptr
Thanks! Addressed comment. https://codereview.chromium.org/1825823002/diff/20001/base/allocator/allocato... File base/allocator/allocator_shim_win.cc (right): https://codereview.chromium.org/1825823002/diff/20001/base/allocator/allocato... base/allocator/allocator_shim_win.cc:47: return _crtheap != NULL; On 2016/03/29 20:23:32, scottmg wrote: > NULL -> nullptr Done.
chrome/app LGTM
manzagop@chromium.org changed reviewers: + thestig@chromium.org
Hi thestig@! Could you have a look for base/ ownership? Thanks! Pierre
lgtm !
On 2016/03/29 21:40:18, Lei Zhang wrote: > lgtm ! Thanks for the review!
The CQ bit was checked by manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1825823002/#ps40001 (title: "Address Scott's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1825823002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1825823002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/03/29 22:15:06, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) I've merged (without conflict) and am kicking it again.
The CQ bit was checked by manzagop@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, grt@chromium.org, scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/1825823002/#ps60001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1825823002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1825823002/60001
Message was sent while issue was closed.
Description was changed from ========== Cleanup: LFH only needs to be enabled on XP/2003 BUG=579196 ========== to ========== Cleanup: LFH only needs to be enabled on XP/2003 BUG=579196 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Cleanup: LFH only needs to be enabled on XP/2003 BUG=579196 ========== to ========== Cleanup: LFH only needs to be enabled on XP/2003 BUG=579196 Committed: https://crrev.com/aa3cff336c91fbe2cd8cc6254736564d95c5b230 Cr-Commit-Position: refs/heads/master@{#383983} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/aa3cff336c91fbe2cd8cc6254736564d95c5b230 Cr-Commit-Position: refs/heads/master@{#383983}
Message was sent while issue was closed.
wfh@chromium.org changed reviewers: + wfh@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1825823002/diff/60001/base/allocator/allocato... File base/allocator/allocator_shim_win.cc (left): https://codereview.chromium.org/1825823002/diff/60001/base/allocator/allocato... base/allocator/allocator_shim_win.cc:47: if (_crtheap == NULL) this code is already going with the move to VS2015... can you include base/allocator owners on changes in allocator rather than going up to base/ thanks!
Message was sent while issue was closed.
https://codereview.chromium.org/1825823002/diff/60001/base/allocator/allocato... File base/allocator/allocator_shim_win.cc (left): https://codereview.chromium.org/1825823002/diff/60001/base/allocator/allocato... base/allocator/allocator_shim_win.cc:47: if (_crtheap == NULL) On 2016/03/30 22:36:36, Will Harris wrote: > this code is already going with the move to VS2015... > > can you include base/allocator owners on changes in allocator rather than going > up to base/ thanks! Sorry, I wasn't sure how to tradeoff reviewer count/specificity (I already needed a base/ reviewer for base/process which doesn't have an owners, and this seemed harmless). Thanks for the heads up, will adjust!
Message was sent while issue was closed.
On 2016/03/31 13:04:46, manzagop wrote: > https://codereview.chromium.org/1825823002/diff/60001/base/allocator/allocato... > File base/allocator/allocator_shim_win.cc (left): > > https://codereview.chromium.org/1825823002/diff/60001/base/allocator/allocato... > base/allocator/allocator_shim_win.cc:47: if (_crtheap == NULL) > On 2016/03/30 22:36:36, Will Harris wrote: > > this code is already going with the move to VS2015... > > > > can you include base/allocator owners on changes in allocator rather than > going > > up to base/ thanks! > > Sorry, I wasn't sure how to tradeoff reviewer count/specificity (I already > needed a base/ reviewer for base/process which doesn't have an owners, and this > seemed harmless). > > Thanks for the heads up, will adjust! No probs! Allocator is in the middle of a bit of flux at the moment that's all, lots of CLs flying around. No problem at all. Thanks for the reply! :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
