|
|
Created:
5 years, 1 month ago by phoglund Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionTemporarily disable VERIFY while bug is investigated.
This breaks some client apps in annoying ways, so disable for now.
BUG=webrtc:4776
Committed: https://crrev.com/4dd7a653b5011495f4c805461bad33405c1fb1b8
Cr-Commit-Position: refs/heads/master@{#10693}
Patch Set 1 #
Total comments: 3
Patch Set 2 : #Messages
Total messages: 18 (5 generated)
phoglund@webrtc.org changed reviewers: + perkj@webrtc.org
perkj@webrtc.org changed reviewers: + glaznev@webrtc.org, hbos@webrtc.org
https://codereview.webrtc.org/1461513003/diff/1/talk/app/webrtc/videosource.cc File talk/app/webrtc/videosource.cc (right): https://codereview.webrtc.org/1461513003/diff/1/talk/app/webrtc/videosource.c... talk/app/webrtc/videosource.cc:465: // TODO(phoglund): Temporarily disabled VERIFY due to issue 4776. TODO(hbos) ? webrtc:4776
I think I read a rule somewhere that you should not put TODOs on other people, but I'm fine with this :) https://codereview.webrtc.org/1461513003/diff/1/talk/app/webrtc/videosource.cc File talk/app/webrtc/videosource.cc (right): https://codereview.webrtc.org/1461513003/diff/1/talk/app/webrtc/videosource.c... talk/app/webrtc/videosource.cc:465: // TODO(phoglund): Temporarily disabled VERIFY due to issue 4776. On 2015/11/18 15:12:20, perkj1 wrote: > TODO(hbos) ? webrtc:4776 Done.
While the verify check causes a crash in some client apps, I don't think we are fully aware of the consequences of removing it. What causes it to be triggered may also cause other problems. How about using ASSERT instead of VERIFY? (That's like using DCHECK instead of CHECK, right?) https://codereview.webrtc.org/1461513003/diff/1/talk/app/webrtc/videosource.cc File talk/app/webrtc/videosource.cc (right): https://codereview.webrtc.org/1461513003/diff/1/talk/app/webrtc/videosource.c... talk/app/webrtc/videosource.cc:465: // TODO(phoglund): Temporarily disabled VERIFY due to issue 4776. On 2015/11/18 15:12:20, perkj1 wrote: > TODO(hbos) ? webrtc:4776 Yeah, assign it to me.
On 2015/11/18 15:29:14, hbos wrote: > While the verify check causes a crash in some client apps, I don't think we are > fully aware of the consequences of removing it. What causes it to be triggered > may also cause other problems. > Yes, but apparently it worked earlier when the check was off (which was the default with the old build system). > How about using ASSERT instead of VERIFY? (That's like using DCHECK instead of > CHECK, right?) > The reason I'm disabling it is because it is annoying to client devs. Replacing it with an ASSERT will make no difference whatsoever. Think of this as an overreaching DCHECK what we're removing temporarily. The only difference with VERIFY is that the VERIFY preserves the expression in it whereas ASSERT expands to (void)0. Replacing VERIFY with ASSERT would completely break the code in release mode. > https://codereview.webrtc.org/1461513003/diff/1/talk/app/webrtc/videosource.cc > File talk/app/webrtc/videosource.cc (right): > > https://codereview.webrtc.org/1461513003/diff/1/talk/app/webrtc/videosource.c... > talk/app/webrtc/videosource.cc:465: // TODO(phoglund): Temporarily disabled > VERIFY due to issue 4776. > On 2015/11/18 15:12:20, perkj1 wrote: > > TODO(hbos) ? webrtc:4776 > > Yeah, assign it to me.
OK. lgtm
The CQ bit was checked by phoglund@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1461513003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461513003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1807)
Alex or Per: need owner stamp, feel free to land if lgty
On 2015/11/18 16:59:19, phoglund wrote: > Alex or Per: need owner stamp, feel free to land if lgty lgtm
The CQ bit was checked by perkj@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1461513003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461513003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4dd7a653b5011495f4c805461bad33405c1fb1b8 Cr-Commit-Position: refs/heads/master@{#10693} |