Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2002073002:
media: Replace wtf/Assertions.h macros in favor of base/logging.h macros (Closed)
|
Created:
4 years, 7 months ago by Srirama Modified:
4 years, 6 months ago Reviewers:
wolenetz CC:
chromium-reviews, blink-reviews, haraken, mlamouri+watch-blink_chromium.org, eric.carlson_apple.com, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Replace wtf/Assertions.h macros in favor of base/logging.h macros
Replaced wtf/Assertions.h macros in favor of
base/logging.h macros in mediasource module.
WTF_LOG -> DVLOG
ASSERT -> DCHECK
ASSERT_NOT_REACHED -> NOTREACHED
BUG=596522
Committed: https://crrev.com/df4ecdea27c48221351a258cdff96b51ef1d01f6
Cr-Commit-Position: refs/heads/master@{#397902}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Rebase and fix test failures #
Messages
Total messages: 28 (13 generated)
Description was changed from ========== fix BUG= ========== to ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replaced wtf/Assertions.h macros in favor of base/logging.h macros in mediasource module. WTF_LOG -> DVLOG ASSERT -> DCHECK BUG=596522 ==========
Description was changed from ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replaced wtf/Assertions.h macros in favor of base/logging.h macros in mediasource module. WTF_LOG -> DVLOG ASSERT -> DCHECK BUG=596522 ========== to ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replaced wtf/Assertions.h macros in favor of base/logging.h macros in mediasource module. WTF_LOG -> DVLOG ASSERT -> DCHECK BUG=596522 ==========
Description was changed from ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replaced wtf/Assertions.h macros in favor of base/logging.h macros in mediasource module. WTF_LOG -> DVLOG ASSERT -> DCHECK BUG=596522 ========== to ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replaced wtf/Assertions.h macros in favor of base/logging.h macros in mediasource module. WTF_LOG -> DVLOG ASSERT -> DCHECK ASSERT_NOT_REACHED -> NOTREACHED BUG=596522 ==========
srirama.m@samsung.com changed reviewers: + wolenetz@chromium.org
PTAL https://codereview.chromium.org/2002073002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/2002073002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:767: DCHECK_EQ(size, 0u); Removed assert for "data" as we are already checking in below if condition.
On 2016/05/23 13:46:18, Srirama wrote: > PTAL > > https://codereview.chromium.org/2002073002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): > > https://codereview.chromium.org/2002073002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:767: > DCHECK_EQ(size, 0u); > Removed assert for "data" as we are already checking in below if condition. Gentle reminder.
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2002073002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2002073002/1
Looks pretty good though % a nit and especially the (IIUC) broken DCHECK_EQ(size, 0u) new condition. For the latter, I kicked off a CQ dry run just now so that we can confirm that the layout tests fail as they should for patch set 1. If they don't, I'd be concerned why. https://codereview.chromium.org/2002073002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/2002073002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:633: << logTrackTypeStr << " Track " << trackBase << "trackId=" << trackBase->trackId() << " id=" nit: indentation https://codereview.chromium.org/2002073002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:767: DCHECK_EQ(size, 0u); On 2016/05/23 13:46:18, Srirama wrote: > Removed assert for "data" as we are already checking in below if condition. The original condition was correct: Caller either passes data=NULL and size=0 or a non-null DATA pointer (with a presumably valid size). In the common case, this DCHECK_EQ of just size always being 0 will now fail incorrectly. Did the layout tests pass on CQ dry run with this (IIUC) broken new condition? If so, we need to fix that too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
https://codereview.chromium.org/2002073002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): https://codereview.chromium.org/2002073002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:633: << logTrackTypeStr << " Track " << trackBase << "trackId=" << trackBase->trackId() << " id=" On 2016/05/31 17:46:18, wolenetz_slow_reviews wrote: > nit: indentation Done. https://codereview.chromium.org/2002073002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:767: DCHECK_EQ(size, 0u); On 2016/05/31 17:46:18, wolenetz_slow_reviews wrote: > On 2016/05/23 13:46:18, Srirama wrote: > > Removed assert for "data" as we are already checking in below if condition. > > The original condition was correct: > Caller either passes data=NULL and size=0 or a non-null DATA pointer (with a > presumably valid size). In the common case, this DCHECK_EQ of just size always > being 0 will now fail incorrectly. Did the layout tests pass on CQ dry run with > this (IIUC) broken new condition? If so, we need to fix that too. Done.
On 2016/06/01 11:39:35, Srirama wrote: > https://codereview.chromium.org/2002073002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp (right): > > https://codereview.chromium.org/2002073002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:633: << > logTrackTypeStr << " Track " << trackBase << "trackId=" << trackBase->trackId() > << " id=" > On 2016/05/31 17:46:18, wolenetz_slow_reviews wrote: > > nit: indentation > > Done. > > https://codereview.chromium.org/2002073002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/mediasource/SourceBuffer.cpp:767: > DCHECK_EQ(size, 0u); > On 2016/05/31 17:46:18, wolenetz_slow_reviews wrote: > > On 2016/05/23 13:46:18, Srirama wrote: > > > Removed assert for "data" as we are already checking in below if condition. > > > > The original condition was correct: > > Caller either passes data=NULL and size=0 or a non-null DATA pointer (with a > > presumably valid size). In the common case, this DCHECK_EQ of just size always > > being 0 will now fail incorrectly. Did the layout tests pass on CQ dry run > with > > this (IIUC) broken new condition? If so, we need to fix that too. > > Done. Gentle reminder.
lgtm Thanks for doing this. I'll send it to CQ now.
The CQ bit was checked by wolenetz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2002073002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wolenetz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2002073002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by srirama.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2002073002/20001
Message was sent while issue was closed.
Description was changed from ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replaced wtf/Assertions.h macros in favor of base/logging.h macros in mediasource module. WTF_LOG -> DVLOG ASSERT -> DCHECK ASSERT_NOT_REACHED -> NOTREACHED BUG=596522 ========== to ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replaced wtf/Assertions.h macros in favor of base/logging.h macros in mediasource module. WTF_LOG -> DVLOG ASSERT -> DCHECK ASSERT_NOT_REACHED -> NOTREACHED BUG=596522 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replaced wtf/Assertions.h macros in favor of base/logging.h macros in mediasource module. WTF_LOG -> DVLOG ASSERT -> DCHECK ASSERT_NOT_REACHED -> NOTREACHED BUG=596522 ========== to ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replaced wtf/Assertions.h macros in favor of base/logging.h macros in mediasource module. WTF_LOG -> DVLOG ASSERT -> DCHECK ASSERT_NOT_REACHED -> NOTREACHED BUG=596522 Committed: https://crrev.com/df4ecdea27c48221351a258cdff96b51ef1d01f6 Cr-Commit-Position: refs/heads/master@{#397902} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/df4ecdea27c48221351a258cdff96b51ef1d01f6 Cr-Commit-Position: refs/heads/master@{#397902} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
