Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1988753002:
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 CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, gasubic, mlamouri+watch-blink_chromium.org, philipj_slow, nessy, vcarbune.chromium 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
Replace WTF_LOG macros with DVLOG in HTMLMediaElement.cpp
BUG=596522
Committed: https://crrev.com/0dbdb7c7b7e7aebe2ab71db16c294699c59d50e1
Cr-Commit-Position: refs/heads/master@{#394742}
Patch Set 1 #
Total comments: 2
Patch Set 2 : address comments #Patch Set 3 : address comments #
Total comments: 12
Patch Set 4 : address comments #Patch Set 5 : Typecast 'this' pointer to 'void*' for printing pointer value #Patch Set 6 : Typecast WebString to String in logs #Patch Set 7 : Remove unnecessary LOG_DISABLED macro #
Total comments: 2
Messages
Total messages: 29 (12 generated)
Description was changed from ========== fix BUG= ========== to ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Repalce WTF_LOG macros with DVLOG in HTMLMediaElement.cpp BUG=596522 ==========
srirama.m@samsung.com changed reviewers: + fs@opera.com
PTAL, there are few more places to change, i will do it based on the decision for the below comment. https://codereview.chromium.org/1988753002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1988753002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:436: DVLOG(1) << "HTMLMediaElement::HTMLMediaElement(" << this << ")"; Should we simplify it by using ":" instead of braces or just keep it as it is?
Personally I'm not a big fan of these types of logging statements, but if others find it useful I guess we can keep them around. Maybe the "level" should be higher than "1" though? https://codereview.chromium.org/1988753002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1988753002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:436: DVLOG(1) << "HTMLMediaElement::HTMLMediaElement(" << this << ")"; On 2016/05/17 at 13:27:37, Srirama wrote: > Should we simplify it by using ":" instead of braces or just keep it as it is? Maybe just have "HTMLMediaElement(" << this << ")" (i.e drop the "HTMLMediaElement::"), since I believe D/VLOG includes file and line#, so it's already quite verbose. Don't know if the additional classname prefix is of much use. Parenthesis are probably fine to keep given what is being logged.
Description was changed from ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Repalce WTF_LOG macros with DVLOG in HTMLMediaElement.cpp BUG=596522 ========== to ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replace WTF_LOG macros with DVLOG in HTMLMediaElement.cpp BUG=596522 ==========
Patchset #2 (id:20001) has been deleted
On 2016/05/17 13:59:07, fs wrote: > Personally I'm not a big fan of these types of logging statements, but if others > find it useful I guess we can keep them around. Maybe the "level" should be > higher than "1" though? Hmm, probably useful for occasional debugging. You want to make the level higher so that it will be less verbose ? > https://codereview.chromium.org/1988753002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1988753002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:436: DVLOG(1) << > "HTMLMediaElement::HTMLMediaElement(" << this << ")"; > On 2016/05/17 at 13:27:37, Srirama wrote: > > Should we simplify it by using ":" instead of braces or just keep it as it is? > > Maybe just have "HTMLMediaElement(" << this << ")" (i.e drop the > "HTMLMediaElement::"), since I believe D/VLOG includes file and line#, so it's > already quite verbose. Don't know if the additional classname prefix is of much > use. > > Parenthesis are probably fine to keep given what is being logged. ok, i tested the logs and it is printing tag name (VIDEO) instead of pointer. But printing pointer looks more useful as we can differentiate the media element in case of multiple elements.
On 2016/05/18 at 11:22:31, srirama.m wrote: > On 2016/05/17 13:59:07, fs wrote: > > Personally I'm not a big fan of these types of logging statements, but if others > > find it useful I guess we can keep them around. Maybe the "level" should be > > higher than "1" though? > > Hmm, probably useful for occasional debugging. For "occasional" debugging you can quite easily add the required output (temporarily) when you need it. > You want to make the level higher so that it will be less verbose ? Yes. Don't know how much higher, but maybe... *draws number from hat* ...3? > > > https://codereview.chromium.org/1988753002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > > > https://codereview.chromium.org/1988753002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:436: DVLOG(1) << > > "HTMLMediaElement::HTMLMediaElement(" << this << ")"; > > On 2016/05/17 at 13:27:37, Srirama wrote: > > > Should we simplify it by using ":" instead of braces or just keep it as it is? > > > > Maybe just have "HTMLMediaElement(" << this << ")" (i.e drop the > > "HTMLMediaElement::"), since I believe D/VLOG includes file and line#, so it's > > already quite verbose. Don't know if the additional classname prefix is of much > > use. > > > > Parenthesis are probably fine to keep given what is being logged. > > ok, i tested the logs and it is printing tag name (VIDEO) instead of pointer. But printing pointer looks more useful as we can differentiate the media element in case of multiple elements. Yes, printing the pointer value does feel more useful. The tag name might make sense in the constructor log, but likely no value in repeating it...
On 2016/05/18 11:29:31, fs wrote: > On 2016/05/18 at 11:22:31, srirama.m wrote: > > On 2016/05/17 13:59:07, fs wrote: > > > Personally I'm not a big fan of these types of logging statements, but if > others > > > find it useful I guess we can keep them around. Maybe the "level" should be > > > higher than "1" though? > > > > Hmm, probably useful for occasional debugging. > > For "occasional" debugging you can quite easily add the required output > (temporarily) when you need it. Agreed, probably we can remove most of the function entry point logs. I will check it and remove it in future. > > > You want to make the level higher so that it will be less verbose ? > > Yes. Don't know how much higher, but maybe... *draws number from hat* ...3? > Ok, 3 should be fine. > > > > > > https://codereview.chromium.org/1988753002/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > > > > > > https://codereview.chromium.org/1988753002/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:436: DVLOG(1) << > > > "HTMLMediaElement::HTMLMediaElement(" << this << ")"; > > > On 2016/05/17 at 13:27:37, Srirama wrote: > > > > Should we simplify it by using ":" instead of braces or just keep it as it > is? > > > > > > Maybe just have "HTMLMediaElement(" << this << ")" (i.e drop the > > > "HTMLMediaElement::"), since I believe D/VLOG includes file and line#, so > it's > > > already quite verbose. Don't know if the additional classname prefix is of > much > > > use. > > > > > > Parenthesis are probably fine to keep given what is being logged. > > > > ok, i tested the logs and it is printing tag name (VIDEO) instead of pointer. > But printing pointer looks more useful as we can differentiate the media element > in case of multiple elements. > > Yes, printing the pointer value does feel more useful. The tag name might make > sense in the constructor log, but likely no value in repeating it... Removed it in all the places except a few where it makes sense.
> > > ok, i tested the logs and it is printing tag name (VIDEO) instead of pointer. > > But printing pointer looks more useful as we can differentiate the media element > > in case of multiple elements. > > > > Yes, printing the pointer value does feel more useful. The tag name might make > > sense in the constructor log, but likely no value in repeating it... > > Removed it in all the places except a few where it makes sense. So no "this pointer context"? https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:97: #define LOG_LEVEL 3 LOG_LEVEL is a pretty generic name, so clashes doesn't seem unlikely. Maybe qualify/prefix with something that reduces the risk (MEDIA_LOG_LEVEL?) https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2333: DVLOG(LOG_LEVEL) << "audioTrackChanged trackId= " << trackId << " enabled=" << enabled; boolString(enabled)? https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2360: << "', '" << label.utf8().data() << "', '" << language.utf8().data() << "', " << enabled << ")"; Ditto. https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2404: << "', '" << label.utf8().data() << "', '" << language.utf8().data() << "', " << selected << ")"; boolString(selected) https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2643: #if !LOG_DISABLED Not sure how useful this is now, but that can be cleaned up separately. https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2882: DVLOG(LOG_LEVEL) << "durationChanged(" << duration << ", " << requestSeek << ")"; boolString?
I removed it because I couldn't find a way to log the pointer. Is there anyway we can still log the pointer value? https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:97: #define LOG_LEVEL 3 On 2016/05/18 13:24:36, fs wrote: > LOG_LEVEL is a pretty generic name, so clashes doesn't seem unlikely. Maybe > qualify/prefix with something that reduces the risk (MEDIA_LOG_LEVEL?) Done. https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2333: DVLOG(LOG_LEVEL) << "audioTrackChanged trackId= " << trackId << " enabled=" << enabled; On 2016/05/18 13:24:36, fs wrote: > boolString(enabled)? Done. https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2360: << "', '" << label.utf8().data() << "', '" << language.utf8().data() << "', " << enabled << ")"; On 2016/05/18 13:24:36, fs wrote: > Ditto. Done. https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2404: << "', '" << label.utf8().data() << "', '" << language.utf8().data() << "', " << selected << ")"; On 2016/05/18 13:24:36, fs wrote: > boolString(selected) Done. https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2643: #if !LOG_DISABLED On 2016/05/18 13:24:36, fs wrote: > Not sure how useful this is now, but that can be cleaned up separately. Acknowledged. https://codereview.chromium.org/1988753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2882: DVLOG(LOG_LEVEL) << "durationChanged(" << duration << ", " << requestSeek << ")"; On 2016/05/18 13:24:36, fs wrote: > boolString? Done.
On 2016/05/18 at 13:55:01, srirama.m wrote: > I removed it because I couldn't find a way to log the pointer. Is there anyway we can still log the pointer value? There's an operator<< overload for Node* (in Node.h) that I assume is what "catches" these. I guess we could try having a local operator<< overload for HTMLMediaElement* (that casts to void* or something before printing) or something similar. Or just add a helper that "unwraps" to a void*. Or any decent variant thereof really. I see that String/AtomicString has operator<< overloads too, so all the .utf8().data() and .ascii().data() uses can be simplified too.
Patchset #5 (id:100001) has been deleted
On 2016/05/18 14:12:33, fs wrote: > On 2016/05/18 at 13:55:01, srirama.m wrote: > > I removed it because I couldn't find a way to log the pointer. Is there anyway > we can still log the pointer value? > > There's an operator<< overload for Node* (in Node.h) that I assume is what > "catches" these. I guess we could try having a local operator<< overload for > HTMLMediaElement* (that casts to void* or something before printing) or > something similar. Or just add a helper that "unwraps" to a void*. Or any decent > variant thereof really. > > I see that String/AtomicString has operator<< overloads too, so all the > .utf8().data() and .ascii().data() uses can be simplified too. Done, used (void*) and it works fine, haven't tried other approaches but looks like they may crash during destructor logging(right now it is crashing trying to access tagname from node). Removed unnecessary utf8() and ascii() conversions except the places where WebString is used.
lgtm > Removed unnecessary utf8() and ascii() conversions except the places where WebString is used. I think you could "get" those too by "casting" to String/AtomicString (which should be a cheap operation). Do as you please.
The CQ bit was checked by srirama.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com Link to the patchset: https://codereview.chromium.org/1988753002/#ps140001 (title: "Typecast WebString to String in logs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988753002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988753002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by srirama.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com Link to the patchset: https://codereview.chromium.org/1988753002/#ps160001 (title: "Remove unnecessary LOG_DISABLED macro")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988753002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988753002/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replace WTF_LOG macros with DVLOG in HTMLMediaElement.cpp BUG=596522 ========== to ========== media: Replace wtf/Assertions.h macros in favor of base/logging.h macros Replace WTF_LOG macros with DVLOG in HTMLMediaElement.cpp BUG=596522 Committed: https://crrev.com/0dbdb7c7b7e7aebe2ab71db16c294699c59d50e1 Cr-Commit-Position: refs/heads/master@{#394742} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0dbdb7c7b7e7aebe2ab71db16c294699c59d50e1 Cr-Commit-Position: refs/heads/master@{#394742}
Message was sent while issue was closed.
wolenetz@chromium.org changed reviewers: + wolenetz@chromium.org
Message was sent while issue was closed.
Perhaps I'm confused, but the commit message somewhat implies that not just
WTF_LOG -> DVLOG is done here, but also other base/logging.h macros. Are
NOTREACHED(),{D}VLOG(...), etc. changes planned for HTMLMediaElement in a later
change?
https://codereview.chromium.org/1988753002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
https://codereview.chromium.org/1988753002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:899:
ASSERT_NOT_REACHED();
nit: NOTREACHED() ?
https://codereview.chromium.org/1988753002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:905:
ASSERT(m_srcObject);
nit: DCHECK(...) ?
Message was sent while issue was closed.
On 2016/05/19 17:43:36, wolenetz wrote:
> Perhaps I'm confused, but the commit message somewhat implies that not just
> WTF_LOG -> DVLOG is done here, but also other base/logging.h macros. Are
> NOTREACHED(),{D}VLOG(...), etc. changes planned for HTMLMediaElement in a
later
> change?
>
>
https://codereview.chromium.org/1988753002/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
>
>
https://codereview.chromium.org/1988753002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:899:
> ASSERT_NOT_REACHED();
> nit: NOTREACHED() ?
>
>
https://codereview.chromium.org/1988753002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:905:
> ASSERT(m_srcObject);
> nit: DCHECK(...) ?
There are too many WTF_LOG messages here, that i forgot about other macros. I
will upload those changes tomorrow.
|
