Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1996603002:
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, 7 months ago Reviewers:
wolenetz CC:
chromium-reviews, feature-media-reviews_chromium.org, mlamouri+watch-blink_chromium.org, philipj_slow, eric.carlson_apple.com, haraken, blink-reviews 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.cpp.
WTF_LOG -> DVLOG
ASSERT -> DCHECK
ASSERT_NOT_REACHED -> NOTREACHED
BUG=596522
Committed: https://crrev.com/bf2181075f253789624f1a76c199182016da9c79
Cr-Commit-Position: refs/heads/master@{#395261}
Patch Set 1 #
Total comments: 16
Patch Set 2 : address comments #
Total comments: 4
Patch Set 3 : address nits #Messages
Total messages: 18 (9 generated)
Description was changed from ========== fix BUG= ========== to ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replace WTF_LOG macros with DVLOG in mediasource module. BUG=596522 ==========
srirama.m@samsung.com changed reviewers: + wolenetz@chromium.org
PTAL
https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:54: #define MEDIA_SOURCE_LOG_LEVEL 3 Is 3 fine to make it less verbose? or should we make it 1?
https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:54: #define MEDIA_SOURCE_LOG_LEVEL 3 On 2016/05/19 15:00:22, Srirama wrote: > Is 3 fine to make it less verbose? or should we make it 1? 3 is fine. It's what I use personally when debugging the engine backing this specific module :) https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:107: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << "MediaSource " << this; Just double-checking: does "this" not need the (void*) cast like was used in HTMLMediaElement (is there no conflicting operator<< in MediaSource inheritance?) https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:118: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << "throwDOMException: error=" << error << " msg=" << message; Is __FUNCTION__ usable here as a replacement (here and below), and can we please include "this" here, too? For example: DVLOG(...) << __FUNCTION__ << "(error=" << error << ", message=" << message << ")" << this; https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:165: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << "addSourceBuffer(" << type << ") " << this << " -> " << buffer; Ditto on the double-check for |buffer| here please. https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:171: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << "removeSourceBuffer() " << this; nit: now seems like a good time to add |buffer| to this log message. https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:476: ASSERT_NOT_REACHED(); // IDL enforcement should prevent this case. NOTREACHED() ? https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:606: ASSERT_NOT_REACHED(); nit: NOTREACHED() ?
Patchset #2 (id:20001) has been deleted
Description was changed from ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replace WTF_LOG macros with DVLOG in mediasource module. 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.cpp. WTF_LOG -> DVLOG ASSERT -> DCHECK ASSERT_NOT_REACHED -> NOTREACHED 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.cpp. 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.cpp. WTF_LOG -> DVLOG ASSERT -> DCHECK ASSERT_NOT_REACHED -> NOTREACHED BUG=596522 ==========
https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:107: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << "MediaSource " << this; On 2016/05/19 17:53:33, wolenetz wrote: > Just double-checking: does "this" not need the (void*) cast like was used in > HTMLMediaElement (is there no conflicting operator<< in MediaSource > inheritance?) It doesn't need, I verified and it is printing the pointer. https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:118: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << "throwDOMException: error=" << error << " msg=" << message; On 2016/05/19 17:53:33, wolenetz wrote: > Is __FUNCTION__ usable here as a replacement (here and below), and can we please > include "this" here, too? For example: > DVLOG(...) << __FUNCTION__ << "(error=" << error << ", message=" << message << > ")" << this; Not able to print "this" here as it is a static function https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:165: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << "addSourceBuffer(" << type << ") " << this << " -> " << buffer; On 2016/05/19 17:53:33, wolenetz wrote: > Ditto on the double-check for |buffer| here please. I think you want the buffer pointer to be printed and if so, it is printing the pointer. https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:171: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << "removeSourceBuffer() " << this; On 2016/05/19 17:53:33, wolenetz wrote: > nit: now seems like a good time to add |buffer| to this log message. Done. https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:476: ASSERT_NOT_REACHED(); // IDL enforcement should prevent this case. On 2016/05/19 17:53:33, wolenetz wrote: > NOTREACHED() ? Done. https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:606: ASSERT_NOT_REACHED(); On 2016/05/19 17:53:33, wolenetz wrote: > nit: NOTREACHED() ? Done.
lgtm % nits https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:118: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << "throwDOMException: error=" << error << " msg=" << message; On 2016/05/20 08:31:48, Srirama wrote: > On 2016/05/19 17:53:33, wolenetz wrote: > > Is __FUNCTION__ usable here as a replacement (here and below), and can we > please > > include "this" here, too? For example: > > DVLOG(...) << __FUNCTION__ << "(error=" << error << ", message=" << message << > > ")" << this; > > Not able to print "this" here as it is a static function Acknowledged. https://codereview.chromium.org/1996603002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:165: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << "addSourceBuffer(" << type << ") " << this << " -> " << buffer; On 2016/05/20 08:31:48, Srirama wrote: > On 2016/05/19 17:53:33, wolenetz wrote: > > Ditto on the double-check for |buffer| here please. > > I think you want the buffer pointer to be printed and if so, it is printing the > pointer. Acknowledged. https://codereview.chromium.org/1996603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/1996603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:171: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << __FUNCTION__ << "(" << this << ") -> " << buffer; nit: swap |this| and |buffer| placement in the log for consistency. https://codereview.chromium.org/1996603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:240: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << __FUNCTION__ << "(" << type << ") -> false (empty input)"; nit: In Chromium, |this| shouldn't make a difference in this method's behavior, but including |this| in these log messages would help isolate which MSE object among many is involved here in a log trace.
https://codereview.chromium.org/1996603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediasource/MediaSource.cpp (right): https://codereview.chromium.org/1996603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:171: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << __FUNCTION__ << "(" << this << ") -> " << buffer; On 2016/05/20 19:58:15, wolenetz wrote: > nit: swap |this| and |buffer| placement in the log for consistency. Done. https://codereview.chromium.org/1996603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediasource/MediaSource.cpp:240: DVLOG(MEDIA_SOURCE_LOG_LEVEL) << __FUNCTION__ << "(" << type << ") -> false (empty input)"; On 2016/05/20 19:58:15, wolenetz wrote: > nit: In Chromium, |this| shouldn't make a difference in this method's behavior, > but including |this| in these log messages would help isolate which MSE object > among many is involved here in a log trace. Same static function issue here.
The CQ bit was checked by srirama.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/1996603002/#ps60001 (title: "address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996603002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996603002/60001
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.cpp. 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.cpp. WTF_LOG -> DVLOG ASSERT -> DCHECK ASSERT_NOT_REACHED -> NOTREACHED BUG=596522 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
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.cpp. 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.cpp. WTF_LOG -> DVLOG ASSERT -> DCHECK ASSERT_NOT_REACHED -> NOTREACHED BUG=596522 Committed: https://crrev.com/bf2181075f253789624f1a76c199182016da9c79 Cr-Commit-Position: refs/heads/master@{#395261} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bf2181075f253789624f1a76c199182016da9c79 Cr-Commit-Position: refs/heads/master@{#395261} |
