Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2208733004:
Fix crash in NQE: Replace DCHECK by an if-conditional (Closed)
DescriptionFix crash in NQE: Replace DCHECK by an if-conditional
https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tester/builds/2914/steps/browser_tests%20on%20Windows-7-SP1/logs/CrSettingsMainPageTest.All
shows a DCHECK failing in NQE because load timing info
is null.
This CL replaces the DCHECK by an if-conditional.
TBR=bengr@chromium.org
BUG=634240
Committed: https://crrev.com/8322dc34d8b35270fd642fa01d96c127ebe4ac3b
Cr-Commit-Position: refs/heads/master@{#409735}
Patch Set 1 #
Messages
Total messages: 22 (14 generated)
The CQ bit was checked by tbansal@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 ========== Replace DCHECK by a check BUG= ========== to ========== Fix crash in NQE: Replace DCHECK by an if-conditional https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... shows a DCHECK failing in NQE because load timing info is null. This CL replaces the DCHECK by an if-conditional. BUG= ==========
Description was changed from ========== Fix crash in NQE: Replace DCHECK by an if-conditional https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... shows a DCHECK failing in NQE because load timing info is null. This CL replaces the DCHECK by an if-conditional. BUG= ========== to ========== Fix crash in NQE: Replace DCHECK by an if-conditional https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... shows a DCHECK failing in NQE because load timing info is null. This CL replaces the DCHECK by an if-conditional. BUG= ==========
Description was changed from ========== Fix crash in NQE: Replace DCHECK by an if-conditional https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... shows a DCHECK failing in NQE because load timing info is null. This CL replaces the DCHECK by an if-conditional. BUG= ========== to ========== Fix crash in NQE: Replace DCHECK by an if-conditional https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... shows a DCHECK failing in NQE because load timing info is null. This CL replaces the DCHECK by an if-conditional. BUG=634240 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix crash in NQE: Replace DCHECK by an if-conditional https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... shows a DCHECK failing in NQE because load timing info is null. This CL replaces the DCHECK by an if-conditional. BUG=634240 ========== to ========== Fix crash in NQE: Replace DCHECK by an if-conditional https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... shows a DCHECK failing in NQE because load timing info is null. This CL replaces the DCHECK by an if-conditional. TBR=bengr@chromium.org BUG=634240 ==========
The CQ bit was checked by tbansal@chromium.org
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 ========== Fix crash in NQE: Replace DCHECK by an if-conditional https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... shows a DCHECK failing in NQE because load timing info is null. This CL replaces the DCHECK by an if-conditional. TBR=bengr@chromium.org BUG=634240 ========== to ========== Fix crash in NQE: Replace DCHECK by an if-conditional https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... shows a DCHECK failing in NQE because load timing info is null. This CL replaces the DCHECK by an if-conditional. TBR=bengr@chromium.org BUG=634240 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix crash in NQE: Replace DCHECK by an if-conditional https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... shows a DCHECK failing in NQE because load timing info is null. This CL replaces the DCHECK by an if-conditional. TBR=bengr@chromium.org BUG=634240 ========== to ========== Fix crash in NQE: Replace DCHECK by an if-conditional https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... shows a DCHECK failing in NQE because load timing info is null. This CL replaces the DCHECK by an if-conditional. TBR=bengr@chromium.org BUG=634240 Committed: https://crrev.com/8322dc34d8b35270fd642fa01d96c127ebe4ac3b Cr-Commit-Position: refs/heads/master@{#409735} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/8322dc34d8b35270fd642fa01d96c127ebe4ac3b Cr-Commit-Position: refs/heads/master@{#409735}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + bengr@google.com, thakis@chromium.org
Message was sent while issue was closed.
Why was this landed without review?
Message was sent while issue was closed.
On 2016/08/04 15:04:59, Nico wrote: > Why was this landed without review? It was a very safe change. I was using a DHCECK, where I should have been using just an if-conditional. Besides, DCHECKs slows down other developers -- They have to comment out all failing DCHECKs one-by-one when they are testing their code. Please let me know if you think it was not a right decision to land this with TBR.
Message was sent while issue was closed.
On 2016/08/04 17:04:17, tbansal1 wrote: > On 2016/08/04 15:04:59, Nico wrote: > > Why was this landed without review? > > It was a very safe change. I was using a DHCECK, where I should have been using > just > an if-conditional. > > Besides, DCHECKs slows down other developers -- They have to comment > out all failing DCHECKs one-by-one when they are testing their code. Please let > me know if > you think it was not a right decision to land this with TBR. Two things: 1. "TBR" means "to be reviewed" not "without review". If you land a CL as TBR, you still should send out a review email, and the review should still happen, just after submitting. (And comments should be addressed in a follow-up CL.) This didn't happen here. 2. The bottom of https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review... explains when to use TBR. From what I can tell, this CL doesn't qualify for this (I guess fixing a very urgent crasher can qualify too, but this one doesn't sound like it was super frequent.) It's not the end of the world, provided this does get a review after the fact.
Message was sent while issue was closed.
Description was changed from ========== Fix crash in NQE: Replace DCHECK by an if-conditional https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... shows a DCHECK failing in NQE because load timing info is null. This CL replaces the DCHECK by an if-conditional. TBR=bengr@chromium.org BUG=634240 Committed: https://crrev.com/8322dc34d8b35270fd642fa01d96c127ebe4ac3b Cr-Commit-Position: refs/heads/master@{#409735} ========== to ========== Fix crash in NQE: Replace DCHECK by an if-conditional https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tes... shows a DCHECK failing in NQE because load timing info is null. This CL replaces the DCHECK by an if-conditional. TBR=bengr@chromium.org BUG=634240 Committed: https://crrev.com/8322dc34d8b35270fd642fa01d96c127ebe4ac3b Cr-Commit-Position: refs/heads/master@{#409735} ==========
Message was sent while issue was closed.
tbansal@chromium.org changed reviewers: + bengr@chromium.org - bengr@google.com
Message was sent while issue was closed.
On 2016/08/04 17:13:20, Nico wrote: > On 2016/08/04 17:04:17, tbansal1 wrote: > > On 2016/08/04 15:04:59, Nico wrote: > > > Why was this landed without review? > > > > It was a very safe change. I was using a DHCECK, where I should have been > using > > just > > an if-conditional. > > > > Besides, DCHECKs slows down other developers -- They have to comment > > out all failing DCHECKs one-by-one when they are testing their code. Please > let > > me know if > > you think it was not a right decision to land this with TBR. > > Two things: > > 1. "TBR" means "to be reviewed" not "without review". If you land a CL as TBR, > you still should send out a review email, and the review should still happen, > just after submitting. (And comments should be addressed in a follow-up CL.) > This didn't happen here. > > 2. The bottom of > https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review... > explains when to use TBR. From what I can tell, this CL doesn't qualify for this > (I guess fixing a very urgent crasher can qualify too, but this one doesn't > sound like it was super frequent.) > > It's not the end of the world, provided this does get a review after the fact. Thanks, that makes sense. I did publish this CL (see #9 above) with the OWNERS included in the TBR. However, I did not explicitly say "PTAL" in the message. That was my bad. I completely agree that any comments received on this CL should be addressed immediately in the following CL. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
